[PATCH 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[PATCH 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Lee Jones]

The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
devices. It has 2 ports which it can use for either; both SATA, both
PCIe or one of each in any configuration.

Cc: devicetree@vger.kernel.org
Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 .../devicetree/bindings/phy/phy-miphy365x.txt      | 43 ++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/phy-miphy365x.txt

diff --git a/Documentation/devicetree/bindings/phy/phy-miphy365x.txt b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
new file mode 100644
index 0000000..fdfa7ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
@@ -0,0 +1,43 @@
+STMicroelectronics STi MIPHY365x PHY binding
+============================================
+
+This binding describes a miphy device that is used to control PHY hardware
+for SATA and PCIe.
+
+Required properties:
+- compatible: Should be "st,miphy365x-phy"
+- #phy-cells: Should be 2 (See example)
+- reg:	      Address and length of the register set for the device
+- reg-names:  The names of the register addresses corresponding to the
+	      registers filled in "reg".
+- st,syscfg : Should be a phandle of the syscfg node.
+
+Example:
+
+	miphy365x_phy: miphy365x@0 {
+		compatible = "st,miphy365x-phy";
+		#phy-cells = <1>;
+		reg =	<0xfe382000 0x100>,
+			<0xfe38a000 0x100>,
+			<0xfe394000 0x100>,
+			<0xfe804000 0x100>;
+		reg-names = "sata0", "sata1", "pcie0", "pcie1";
+		st,syscfg= <&syscfg_rear>;
+	};
+
+Specifying phy control of devices
+=================================
+
+Device nodes should specify the configuration required in their "phys"
+property, containing a phandle to the miphy device node, a port number
+and a device type.
+
+Example:
+
+#include <dt-bindings/phy/phy-miphy365x.h>
+
+	sata0: sata@fe380000 {
+		...
+		phys	  = <&miphy365x_phy MIPHY_PORT_0 MIPHY_TYPE_SATA>;
+		...
+	};
-- 
1.8.3.2

[PATCH 2/4] phy: miphy365x: Add MiPHY365x header file for DT x Driver defines

[Lee Jones]

This provides the shared header file which will be reference from both
the MiPHY365x driver and its associated Device Tree node(s).

Cc: devicetree@vger.kernel.org
Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 include/dt-bindings/phy/phy-miphy365x.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 include/dt-bindings/phy/phy-miphy365x.h

diff --git a/include/dt-bindings/phy/phy-miphy365x.h b/include/dt-bindings/phy/phy-miphy365x.h
new file mode 100644
index 0000000..8757c02
--- /dev/null
+++ b/include/dt-bindings/phy/phy-miphy365x.h
@@ -0,0 +1,25 @@
+/*
+ * This header provides constants for the phy framework
+ * based on the STMicroelectronics miphy365x.
+ */
+#ifndef _DT_BINDINGS_PHY_MIPHY
+#define _DT_BINDINGS_PHY_MIPHY
+
+/* Supports 16 ports without a datatype change (u8 & 0xF0). */
+#define MIPHY_PORT_0			0
+#define MIPHY_PORT_1			1
+#define MIPHY_PORT_2			2
+#define MIPHY_PORT_3			3
+
+/* Supports 16 types without a datatype change (u8 & 0x0F). */
+#define MIPHY_TYPE_SHIFT		4
+#define MIPHY_TYPE_SATA			(0 << MIPHY_TYPE_SHIFT)
+#define MIPHY_TYPE_PCIE			(1 << MIPHY_TYPE_SHIFT)
+#define MIPHY_TYPE_USB			(2 << MIPHY_TYPE_SHIFT)
+
+#define MIPHY_SATA_PORT0		(MIPHY_TYPE_SATA) | (MIPHY_PORT_0)
+#define MIPHY_SATA_PORT1		(MIPHY_TYPE_SATA) | (MIPHY_PORT_1)
+#define MIPHY_PCIE_PORT0		(MIPHY_TYPE_PCIE) | (MIPHY_PORT_0)
+#define MIPHY_PCIE_PORT1		(MIPHY_TYPE_PCIE) | (MIPHY_PORT_1)
+
+#endif /* _DT_BINDINGS_PHY_MIPHY */
-- 
1.8.3.2

[PATCH 3/4] ARM: DT: STi: Add DT node for MiPHY365x

[Lee Jones]

The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
devices. It has 2 ports which it can use for either; both SATA, both
PCIe or one of each in any configuration.

Cc: devicetree@vger.kernel.org
Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih416-b2020-revE.dts |  6 +++++-
 arch/arm/boot/dts/stih416-b2020.dts      |  6 ++++++
 arch/arm/boot/dts/stih416.dtsi           | 13 +++++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/stih416-b2020-revE.dts b/arch/arm/boot/dts/stih416-b2020-revE.dts
index a874570..dbe67fa 100644
--- a/arch/arm/boot/dts/stih416-b2020-revE.dts
+++ b/arch/arm/boot/dts/stih416-b2020-revE.dts
@@ -32,6 +32,10 @@
 		ethernet1: ethernet@fef08000 {
 			snps,reset-gpio 	= <&PIO0 7>;
 		};
-	};
 
+		miphy365x_phy: miphy365x@0 {
+			st,pcie_tx_pol_inv = <1>;
+			st,sata_gen = "gen3";
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/stih416-b2020.dts b/arch/arm/boot/dts/stih416-b2020.dts
index 276f28d..fd9cbad 100644
--- a/arch/arm/boot/dts/stih416-b2020.dts
+++ b/arch/arm/boot/dts/stih416-b2020.dts
@@ -13,4 +13,10 @@
 	model = "STiH416 B2020";
 	compatible = "st,stih416", "st,stih416-b2020";
 
+	soc {
+		miphy365x_phy: miphy365x@0 {
+			st,pcie_tx_pol_inv = <1>;
+			st,sata_gen = "gen3";
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index 85b8063..9fd8efb 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -9,6 +9,8 @@
 #include "stih41x.dtsi"
 #include "stih416-clock.dtsi"
 #include "stih416-pinctrl.dtsi"
+
+#include <dt-bindings/phy/phy-miphy365x.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/reset-controller/stih416-resets.h>
 / {
@@ -140,5 +142,16 @@
 			clocks		= <&CLK_S_ICN_REG_0>;
 		};
 
+		miphy365x_phy: miphy365x@0 {
+			compatible      = "st,miphy365x-phy";
+			reg        	= <0xfe382000 0x100>,
+					  <0xfe38a000 0x100>,
+					  <0xfe394000 0x100>,
+					  <0xfe804000 0x100>;
+			reg-names  	= "sata0", "sata1", "pcie0", "pcie1";
+
+			#phy-cells 	= <2>;
+			st,syscfg  	= <&syscfg_rear>;
+		};
 	};
 };
-- 
1.8.3.2

Re: [PATCH 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Mark Rutland]

On Wed, Feb 12, 2014 at 04:03:02PM +0000, Lee Jones wrote:
> The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
> devices. It has 2 ports which it can use for either; both SATA, both
> PCIe or one of each in any configuration.
> 
> Cc: devicetree@vger.kernel.org
> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  .../devicetree/bindings/phy/phy-miphy365x.txt      | 43 ++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> 
> diff --git a/Documentation/devicetree/bindings/phy/phy-miphy365x.txt b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> new file mode 100644
> index 0000000..fdfa7ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> @@ -0,0 +1,43 @@
> +STMicroelectronics STi MIPHY365x PHY binding
> +============================================
> +
> +This binding describes a miphy device that is used to control PHY hardware
> +for SATA and PCIe.
> +
> +Required properties:
> +- compatible: Should be "st,miphy365x-phy"
> +- #phy-cells: Should be 2 (See example)

The first example has #phy-cells = <1>.

What do the cells mean? What are the expected values?

> +- reg:	      Address and length of the register set for the device
> +- reg-names:  The names of the register addresses corresponding to the
> +	      registers filled in "reg".

Whenever there is a ${PROP}-names property, there should be a list of
explicit values, and a description of how it relates to ${PROP}. Without
that it's a bit useless.

Please provide an explicit list of expected names here.

I assume here what you want is something like:

- reg: a list of address + length pairs, one for each entry in reg-names
- reg-names: should contain:
  * "sata0" for the sata0 control registers...
  * "sata1" ...
  * "pcie0" ...
  * "pcie1" ...

> +- st,syscfg : Should be a phandle of the syscfg node.

What's this used for?

Cheers,
Mark.

Re: [PATCH 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Lee Jones]

> > The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
> > devices. It has 2 ports which it can use for either; both SATA, both
> > PCIe or one of each in any configuration.
> > 
> > Cc: devicetree@vger.kernel.org
> > Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  .../devicetree/bindings/phy/phy-miphy365x.txt      | 43 ++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/phy/phy-miphy365x.txt b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> > new file mode 100644
> > index 0000000..fdfa7ca
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> > @@ -0,0 +1,43 @@
> > +STMicroelectronics STi MIPHY365x PHY binding
> > +============================================
> > +
> > +This binding describes a miphy device that is used to control PHY hardware
> > +for SATA and PCIe.
> > +
> > +Required properties:
> > +- compatible: Should be "st,miphy365x-phy"
> > +- #phy-cells: Should be 2 (See example)
> 
> The first example has #phy-cells = <1>.

Right, will fix. Should be 2.

> What do the cells mean? What are the expected values?

http://www.spinics.net/lists/arm-kernel/msg307209.html

> > +- reg:	      Address and length of the register set for the device
> > +- reg-names:  The names of the register addresses corresponding to the
> > +	      registers filled in "reg".
> 
> Whenever there is a ${PROP}-names property, there should be a list of
> explicit values, and a description of how it relates to ${PROP}. Without
> that it's a bit useless.
> 
> Please provide an explicit list of expected names here.
> 
> I assume here what you want is something like:
> 
> - reg: a list of address + length pairs, one for each entry in reg-names
> - reg-names: should contain:
>   * "sata0" for the sata0 control registers...
>   * "sata1" ...
>   * "pcie0" ...
>   * "pcie1" ...

Can do.

> > +- st,syscfg : Should be a phandle of the syscfg node.
> 
> What's this used for?

It's used to gain access to the system configuration
registers. Specifically in this case the bits to choose between PCI or
SATA mode.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Mark Rutland]

On Thu, Feb 13, 2014 at 11:03:14AM +0000, Lee Jones wrote:
> > > The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
> > > devices. It has 2 ports which it can use for either; both SATA, both
> > > PCIe or one of each in any configuration.
> > > 
> > > Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > Cc: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
> > > Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > ---
> > >  .../devicetree/bindings/phy/phy-miphy365x.txt      | 43 ++++++++++++++++++++++
> > >  1 file changed, 43 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/phy/phy-miphy365x.txt b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> > > new file mode 100644
> > > index 0000000..fdfa7ca
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> > > @@ -0,0 +1,43 @@
> > > +STMicroelectronics STi MIPHY365x PHY binding
> > > +============================================
> > > +
> > > +This binding describes a miphy device that is used to control PHY hardware
> > > +for SATA and PCIe.
> > > +
> > > +Required properties:
> > > +- compatible: Should be "st,miphy365x-phy"
> > > +- #phy-cells: Should be 2 (See example)
> > 
> > The first example has #phy-cells = <1>.
> 
> Right, will fix. Should be 2.
> 
> > What do the cells mean? What are the expected values?
> 
> http://www.spinics.net/lists/arm-kernel/msg307209.html

Ok. Could that be mentioned in the binding document then?

Cheers,
Mark.
--
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 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Lee Jones]

The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
devices. It has 2 ports which it can use for either; both SATA, both
PCIe or one of each in any configuration.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 .../devicetree/bindings/phy/phy-miphy365x.txt      | 54 ++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/phy-miphy365x.txt

diff --git a/Documentation/devicetree/bindings/phy/phy-miphy365x.txt b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
new file mode 100644
index 0000000..96f269f
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
@@ -0,0 +1,54 @@
+STMicroelectronics STi MIPHY365x PHY binding
+============================================
+
+This binding describes a miphy device that is used to control PHY hardware
+for SATA and PCIe.
+
+Required properties:
+- compatible: Should be "st,miphy365x-phy"
+- #phy-cells: Should be 2 (See second example)
+		First cell is the port number; MIPHY_PORT_{0,1}
+		Second cell is device type; MIPHY_TYPE_{SATA,PCI}
+- reg:	      Address and length of the register set for the device
+- reg-names:  The names of the register addresses corresponding to the
+	      registers filled in "reg"
+		Options are; sata{0,1} and pcie{0,1} (See first example)
+- st,syscfg : Should be a phandle of the system configuration register group
+	      which contain the SATA, PCIe mode setting bits
+
+Optional properties:
+- st,sata-gen	     : Generation of locally attached SATA IP. Expected values
+		       are {1,2,3). If not supplied generation 1 hardware will
+		       be expected
+- st,pcie-tx-pol-inv : Bool property to invert the polarity PCIe Tx (Txn/Txp)
+- st,sata-tx-pol-inv : Bool property to invert the polarity SATA Tx (Txn/Txp)
+
+Example:
+
+	miphy365x_phy: miphy365x@0 {
+		compatible = "st,miphy365x-phy";
+		#phy-cells = <2>;
+		reg =	<0xfe382000 0x100>,
+			<0xfe38a000 0x100>,
+			<0xfe394000 0x100>,
+			<0xfe804000 0x100>;
+		reg-names = "sata0", "sata1", "pcie0", "pcie1";
+		st,syscfg= <&syscfg_rear>;
+	};
+
+Specifying phy control of devices
+=================================
+
+Device nodes should specify the configuration required in their "phys"
+property, containing a phandle to the miphy device node, a port number
+and a device type.
+
+Example:
+
+#include <dt-bindings/phy/phy-miphy365x.h>
+
+	sata0: sata@fe380000 {
+		...
+		phys	  = <&miphy365x_phy MIPHY_PORT_0 MIPHY_TYPE_SATA>;
+		...
+	};
-- 
1.8.3.2

--
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 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Mark Rutland]

On Fri, Feb 14, 2014 at 11:23:53AM +0000, Lee Jones wrote:
> The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
> devices. It has 2 ports which it can use for either; both SATA, both
> PCIe or one of each in any configuration.
> 
> Cc: devicetree@vger.kernel.org
> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  .../devicetree/bindings/phy/phy-miphy365x.txt      | 54 ++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> 
> diff --git a/Documentation/devicetree/bindings/phy/phy-miphy365x.txt b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> new file mode 100644
> index 0000000..96f269f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> @@ -0,0 +1,54 @@
> +STMicroelectronics STi MIPHY365x PHY binding
> +============================================
> +
> +This binding describes a miphy device that is used to control PHY hardware
> +for SATA and PCIe.
> +
> +Required properties:
> +- compatible: Should be "st,miphy365x-phy"
> +- #phy-cells: Should be 2 (See second example)
> +		First cell is the port number; MIPHY_PORT_{0,1}
> +		Second cell is device type; MIPHY_TYPE_{SATA,PCI}

Either this should refer to the header file, or specific values should
be given in the binding document.

> +- reg:	      Address and length of the register set for the device
> +- reg-names:  The names of the register addresses corresponding to the
> +	      registers filled in "reg"
> +		Options are; sata{0,1} and pcie{0,1} (See first example)

How about something like:

- reg: a list of offset + length pairs, one for each entry in reg-names
- reg-names: should contain some of:
  * "sata0" for ...
  * "sata1" for ...
  * "pcie0" for ...
  * "pcie1" for ...

Where ... might just be "the sata port 0 registers"

> +- st,syscfg : Should be a phandle of the system configuration register group
> +	      which contain the SATA, PCIe mode setting bits

I'll assume this is well-defined by some other binding.

> +
> +Optional properties:
> +- st,sata-gen	     : Generation of locally attached SATA IP. Expected values
> +		       are {1,2,3). If not supplied generation 1 hardware will
> +		       be expected
> +- st,pcie-tx-pol-inv : Bool property to invert the polarity PCIe Tx (Txn/Txp)
> +- st,sata-tx-pol-inv : Bool property to invert the polarity SATA Tx (Txn/Txp)

It might just be me, but the phrase "invert the polarity {SATA,PCIe} Tx"
sounds odd. What exactly is being inverted?

> +
> +Example:
> +
> +	miphy365x_phy: miphy365x@0 {
> +		compatible = "st,miphy365x-phy";
> +		#phy-cells = <2>;
> +		reg =	<0xfe382000 0x100>,
> +			<0xfe38a000 0x100>,
> +			<0xfe394000 0x100>,
> +			<0xfe804000 0x100>;
> +		reg-names = "sata0", "sata1", "pcie0", "pcie1";
> +		st,syscfg= <&syscfg_rear>;

Nit: missing space before '='.

> +	};
> +
> +Specifying phy control of devices
> +=================================
> +
> +Device nodes should specify the configuration required in their "phys"
> +property, containing a phandle to the miphy device node, a port number
> +and a device type.
> +
> +Example:
> +
> +#include <dt-bindings/phy/phy-miphy365x.h>
> +
> +	sata0: sata@fe380000 {
> +		...
> +		phys	  = <&miphy365x_phy MIPHY_PORT_0 MIPHY_TYPE_SATA>;
> +		...
> +	};

Is there not a generic phy binding we can point to? It seems a bit
redundant to do this in each phy binding.

Cheers,
Mark.

Re: [PATCH 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Lee Jones]

On Wed, 05 Mar 2014, Mark Rutland wrote:

> On Fri, Feb 14, 2014 at 11:23:53AM +0000, Lee Jones wrote:
> > The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
> > devices. It has 2 ports which it can use for either; both SATA, both
> > PCIe or one of each in any configuration.
> > 
> > Cc: devicetree@vger.kernel.org
> > Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  .../devicetree/bindings/phy/phy-miphy365x.txt      | 54 ++++++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/phy/phy-miphy365x.txt b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> > new file mode 100644
> > index 0000000..96f269f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> > @@ -0,0 +1,54 @@
> > +STMicroelectronics STi MIPHY365x PHY binding
> > +============================================
> > +
> > +This binding describes a miphy device that is used to control PHY hardware
> > +for SATA and PCIe.
> > +
> > +Required properties:
> > +- compatible: Should be "st,miphy365x-phy"
> > +- #phy-cells: Should be 2 (See second example)
> > +		First cell is the port number; MIPHY_PORT_{0,1}
> > +		Second cell is device type; MIPHY_TYPE_{SATA,PCI}
> 
> Either this should refer to the header file, or specific values should
> be given in the binding document.

When you say specific values you mean break out the {,}s?

I thought most people would know what they mean.

> > +- reg:	      Address and length of the register set for the device
> > +- reg-names:  The names of the register addresses corresponding to the
> > +	      registers filled in "reg"
> > +		Options are; sata{0,1} and pcie{0,1} (See first example)
> 
> How about something like:

Same here.

> - reg: a list of offset + length pairs, one for each entry in reg-names
> - reg-names: should contain some of:
>   * "sata0" for ...
>   * "sata1" for ...
>   * "pcie0" for ...
>   * "pcie1" for ...
> 
> Where ... might just be "the sata port 0 registers"

Seems awfully redundant.

> > +- st,syscfg : Should be a phandle of the system configuration register group
> > +	      which contain the SATA, PCIe mode setting bits
> 
> I'll assume this is well-defined by some other binding.

Right.

> > +
> > +Optional properties:
> > +- st,sata-gen	     : Generation of locally attached SATA IP. Expected values
> > +		       are {1,2,3). If not supplied generation 1 hardware will
> > +		       be expected
> > +- st,pcie-tx-pol-inv : Bool property to invert the polarity PCIe Tx (Txn/Txp)
> > +- st,sata-tx-pol-inv : Bool property to invert the polarity SATA Tx (Txn/Txp)
> 
> It might just be me, but the phrase "invert the polarity {SATA,PCIe} Tx"
> sounds odd. What exactly is being inverted?

From the doc (and the only reference of this functionallity):

rx_polarity:
  1: switch rxp/rxn
tx_polarity:
  1: switch txp/txn

If we don't set these registers up to reflect the h/w configuration of
the board, the MiPHY will not work.

> > +
> > +Example:
> > +
> > +	miphy365x_phy: miphy365x@0 {
> > +		compatible = "st,miphy365x-phy";
> > +		#phy-cells = <2>;
> > +		reg =	<0xfe382000 0x100>,
> > +			<0xfe38a000 0x100>,
> > +			<0xfe394000 0x100>,
> > +			<0xfe804000 0x100>;
> > +		reg-names = "sata0", "sata1", "pcie0", "pcie1";
> > +		st,syscfg= <&syscfg_rear>;
> 
> Nit: missing space before '='.

Will fix.

> > +	};
> > +
> > +Specifying phy control of devices
> > +=================================
> > +
> > +Device nodes should specify the configuration required in their "phys"
> > +property, containing a phandle to the miphy device node, a port number
> > +and a device type.
> > +
> > +Example:
> > +
> > +#include <dt-bindings/phy/phy-miphy365x.h>
> > +
> > +	sata0: sata@fe380000 {
> > +		...
> > +		phys	  = <&miphy365x_phy MIPHY_PORT_0 MIPHY_TYPE_SATA>;
> > +		...
> > +	};
> 
> Is there not a generic phy binding we can point to? It seems a bit
> redundant to do this in each phy binding.

Sure, but that wouldn't make much of an example.

  Documentation/devicetree/bindings/phy/phy-bindings.txt

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Lee Jones]

> > > +Specifying phy control of devices
> > > +=================================
> > > +
> > > +Device nodes should specify the configuration required in their "phys"
> > > +property, containing a phandle to the miphy device node, a port number
> > > +and a device type.
> > > +
> > > +Example:
> > > +
> > > +#include <dt-bindings/phy/phy-miphy365x.h>
> > > +
> > > +	sata0: sata@fe380000 {
> > > +		...
> > > +		phys	  = <&miphy365x_phy MIPHY_PORT_0 MIPHY_TYPE_SATA>;
> > > +		...
> > > +	};
> > 
> > Is there not a generic phy binding we can point to? It seems a bit
> > redundant to do this in each phy binding.
> 
> Sure, but that wouldn't make much of an example.
> 
>   Documentation/devicetree/bindings/phy/phy-bindings.txt

Ah sorry, I see what you mean now.

No, not yet. That's the next thing to go up into Mainline.

I will replace this second example with a link when the other
documentation document has been accepted.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Kishon Vijay Abraham I]

Hi,

On Tuesday 17 June 2014 04:53 PM, Lee Jones wrote:
>>> The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
>>> devices. It has 2 ports which it can use for either; both SATA, both
>>> PCIe or one of each in any configuration.
>>
>> I've asked others who wrote multi-phy PHY providers to model each individual
>> PHY as sub-node of the PHY provider. So It's only fair I ask you the same to
>> do. So in this case the dt node should look something like:
>>
>> 	miphy365x_phy: miphy365x@fe382000 {
>> 		compatible = "st,miphy365x-phy";
>> 		#phy-cells = <2>;
>> 		st,syscfg = <&syscfg_rear>;
>> 		channel@0 {
>> 			reg =	<0xfe382000 0x100>, <0xfe394000 0x100>;
>> 			reg-names = "sata", "pcie";
>> 		}
>>
>> 		channel@1{
>> 			reg =	<0xfe38a000 0x100>, <0xfe804000 0x100>;
>> 			reg-names = "sata", "pcie";
>> 		}
>>
>> 	};
> 
> I'm interested to know why you've taken this approach, as it makes the
> code much more complex.  The DT framework goes to the trouble of

It looks to be much closer representation of the hardware in dt. It also
enables to have more control of each individual PHYs. For example, we can have
something like status="disabled" for channels which is disabled.

> converting all addresses to to resources so drivers can easily pull
> them out using platform_get_resource() and friends.  Pushing the reg

right. Can't we use of_address_to_resource here?
> properties down into a child node means that we have to now iterate
> over the sub-nodes and pull them out manually.  This will lead to a

You anyway iterate while creating PHYs based on some constant. Now you have to
iterate over the sub-nodes.
> pretty messy implementation IMHO.


> 
> Can you point me in the direction of previous implementations where you
> have stipulated the same set of constraints please?

ah.. there isn't any. The author of the other multi-phy driver [1] also feels
this will just add to the complexity of the driver.

Maybe we should ask the opinion of others?

[1] -> http://www.spinics.net/lists/linux-sh/msg32087.html
> 

Re: [PATCH 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Lee Jones]

> On Tuesday 17 June 2014 04:53 PM, Lee Jones wrote:
> >>> The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
> >>> devices. It has 2 ports which it can use for either; both SATA, both
> >>> PCIe or one of each in any configuration.
> >>
> >> I've asked others who wrote multi-phy PHY providers to model each individual
> >> PHY as sub-node of the PHY provider. So It's only fair I ask you the same to
> >> do. So in this case the dt node should look something like:
> >>
> >> 	miphy365x_phy: miphy365x@fe382000 {
> >> 		compatible = "st,miphy365x-phy";
> >> 		#phy-cells = <2>;
> >> 		st,syscfg = <&syscfg_rear>;
> >> 		channel@0 {
> >> 			reg =	<0xfe382000 0x100>, <0xfe394000 0x100>;
> >> 			reg-names = "sata", "pcie";
> >> 		}
> >>
> >> 		channel@1{
> >> 			reg =	<0xfe38a000 0x100>, <0xfe804000 0x100>;
> >> 			reg-names = "sata", "pcie";
> >> 		}
> >>
> >> 	};
> > 
> > I'm interested to know why you've taken this approach, as it makes the
> > code much more complex.  The DT framework goes to the trouble of
> 
> It looks to be much closer representation of the hardware in dt. It also
> enables to have more control of each individual PHYs. For example, we can have
> something like status="disabled" for channels which is disabled.

If you wanted to disable the channels in this way, you would either
have to A) add your own code to parse that property or B) have each
channel set itself up as platform device and would probe (or not if
status = "disabled") individually.  If you choose the later option,
the platform resource would also be populated.

> > converting all addresses to to resources so drivers can easily pull
> > them out using platform_get_resource() and friends.  Pushing the reg
> 
> right. Can't we use of_address_to_resource here?

We could, but that would be an extra layer.  We'd be pulling the
address, putting it into a resource, then pulling it from the resource
for use.  If we're going to be pulling addresses out manually, we're
probably better off using of_get_address().  But again, we're just
carrying out functionality which is already provided by the
framework.

> > properties down into a child node means that we have to now iterate
> > over the sub-nodes and pull them out manually.  This will lead to a
> 
> You anyway iterate while creating PHYs based on some constant. Now you have to
> iterate over the sub-nodes.
> > pretty messy implementation IMHO.

This much is true.

> > Can you point me in the direction of previous implementations where you
> > have stipulated the same set of constraints please?
> 
> ah.. there isn't any. The author of the other multi-phy driver [1] also feels
> this will just add to the complexity of the driver.

=:)

> Maybe we should ask the opinion of others?

We could.  I'll CC Arnd as he likes this PHY stuff. :)

> [1] -> http://www.spinics.net/lists/linux-sh/msg32087.html

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Arnd Bergmann]

On Wednesday 18 June 2014 11:04:11 Lee Jones wrote:
> > On Tuesday 17 June 2014 04:53 PM, Lee Jones wrote:
> > >>> The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
> > >>> devices. It has 2 ports which it can use for either; both SATA, both
> > >>> PCIe or one of each in any configuration.
> > >>
> > >> I've asked others who wrote multi-phy PHY providers to model each individual
> > >> PHY as sub-node of the PHY provider. So It's only fair I ask you the same to
> > >> do. So in this case the dt node should look something like:
> > >>
> > >> 	miphy365x_phy: miphy365x@fe382000 {
> > >> 		compatible = "st,miphy365x-phy";
> > >> 		#phy-cells = <2>;
> > >> 		st,syscfg = <&syscfg_rear>;
> > >> 		channel@0 {
> > >> 			reg =	<0xfe382000 0x100>, <0xfe394000 0x100>;
> > >> 			reg-names = "sata", "pcie";
> > >> 		}
> > >>
> > >> 		channel@1{
> > >> 			reg =	<0xfe38a000 0x100>, <0xfe804000 0x100>;
> > >> 			reg-names = "sata", "pcie";
> > >> 		}
> > >>
> > >> 	};
> > > 
> > > I'm interested to know why you've taken this approach, as it makes the
> > > code much more complex.  The DT framework goes to the trouble of
> > 
> > It looks to be much closer representation of the hardware in dt. It also
> > enables to have more control of each individual PHYs. For example, we can have
> > something like status="disabled" for channels which is disabled.
> 
> If you wanted to disable the channels in this way, you would either
> have to A) add your own code to parse that property or B) have each
> channel set itself up as platform device and would probe (or not if
> status = "disabled") individually.  If you choose the later option,
> the platform resource would also be populated.

We have of_device_is_available() and for_each_available_child_of_node()
to check for the status property. This doesn't seem much extra overhead.

> > > converting all addresses to to resources so drivers can easily pull
> > > them out using platform_get_resource() and friends.  Pushing the reg
> > 
> > right. Can't we use of_address_to_resource here?
> 
> We could, but that would be an extra layer.  We'd be pulling the
> address, putting it into a resource, then pulling it from the resource
> for use.  If we're going to be pulling addresses out manually, we're
> probably better off using of_get_address().  But again, we're just
> carrying out functionality which is already provided by the
> framework.

there is also of_ioremap().

> > > properties down into a child node means that we have to now iterate
> > > over the sub-nodes and pull them out manually.  This will lead to a
> > 
> > You anyway iterate while creating PHYs based on some constant. Now you have to
> > iterate over the sub-nodes.
> > > pretty messy implementation IMHO.
> 
> This much is true.
> 
> > > Can you point me in the direction of previous implementations where you
> > > have stipulated the same set of constraints please?
> > 
> > ah.. there isn't any. The author of the other multi-phy driver [1] also feels
> > this will just add to the complexity of the driver.
> 
> =:)
> 
> > Maybe we should ask the opinion of others?
> 
> We could.  I'll CC Arnd as he likes this PHY stuff. :)
> 
> > [1] -> http://www.spinics.net/lists/linux-sh/msg32087.html

Having sub-nodes for each individual PHY managed by a controller seems
very reasonable to me. Making them show up as separate platform devices
seems less useful though.

	Arnd
--
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 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Lee Jones]

> > > > converting all addresses to to resources so drivers can easily pull
> > > > them out using platform_get_resource() and friends.  Pushing the reg
> > > 
> > > right. Can't we use of_address_to_resource here?
> > 
> > We could, but that would be an extra layer.  We'd be pulling the
> > address, putting it into a resource, then pulling it from the resource
> > for use.  If we're going to be pulling addresses out manually, we're
> > probably better off using of_get_address().  But again, we're just
> > carrying out functionality which is already provided by the
> > framework.
> 
> there is also of_ioremap().

Isn't this SPARK only?  And doesn't it require a populated resource?  
Which is what I'm saying is the issue here i.e. we don't have one.

> > > > properties down into a child node means that we have to now iterate
> > > > over the sub-nodes and pull them out manually.  This will lead to a
> > > 
> > > You anyway iterate while creating PHYs based on some constant. Now you have to
> > > iterate over the sub-nodes.
> > > > pretty messy implementation IMHO.
> > 
> > This much is true.
> > 
> > > > Can you point me in the direction of previous implementations where you
> > > > have stipulated the same set of constraints please?
> > > 
> > > ah.. there isn't any. The author of the other multi-phy driver [1] also feels
> > > this will just add to the complexity of the driver.
> > 
> > =:)
> > 
> > > Maybe we should ask the opinion of others?
> > 
> > We could.  I'll CC Arnd as he likes this PHY stuff. :)
> > 
> > > [1] -> http://www.spinics.net/lists/linux-sh/msg32087.html
> 
> Having sub-nodes for each individual PHY managed by a controller seems
> very reasonable to me. Making them show up as separate platform devices
> seems less useful though.

Are there any examples of other nodes with reg properties, but not
compatible strings i.e. ones that aren't probed independently and
aren't platform devices that I can use for reference.  I'm having a
hard time figuring out how to _easily_ obtain indexed addresses
without adding a bunch of new code.  Perhaps if we did something in
the core there would be less overhead overall? 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Arnd Bergmann]

On Tuesday 24 June 2014 13:46:34 Lee Jones wrote:
> > > > > converting all addresses to to resources so drivers can easily pull
> > > > > them out using platform_get_resource() and friends.  Pushing the reg
> > > > 
> > > > right. Can't we use of_address_to_resource here?
> > > 
> > > We could, but that would be an extra layer.  We'd be pulling the
> > > address, putting it into a resource, then pulling it from the resource
> > > for use.  If we're going to be pulling addresses out manually, we're
> > > probably better off using of_get_address().  But again, we're just
> > > carrying out functionality which is already provided by the
> > > framework.
> > 
> > there is also of_ioremap().
> 
> Isn't this SPARK only?  And doesn't it require a populated resource?  
> Which is what I'm saying is the issue here i.e. we don't have one.

Sorry, I meant of_iomap(). I think it's only for historic reaons
that we have both. Probably of_iomap started out on powerpc with
the same intention as the sparc of_ioremap.

of_iomap does not require a resource, just an index.

	Arnd
--
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 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x

[Lee Jones]

On Tue, 24 Jun 2014, Arnd Bergmann wrote:
> On Tuesday 24 June 2014 13:46:34 Lee Jones wrote:
> > > > > > converting all addresses to to resources so drivers can easily pull
> > > > > > them out using platform_get_resource() and friends.  Pushing the reg
> > > > > 
> > > > > right. Can't we use of_address_to_resource here?
> > > > 
> > > > We could, but that would be an extra layer.  We'd be pulling the
> > > > address, putting it into a resource, then pulling it from the resource
> > > > for use.  If we're going to be pulling addresses out manually, we're
> > > > probably better off using of_get_address().  But again, we're just
> > > > carrying out functionality which is already provided by the
> > > > framework.
> > > 
> > > there is also of_ioremap().
> > 
> > Isn't this SPARK only?  And doesn't it require a populated resource?  
> > Which is what I'm saying is the issue here i.e. we don't have one.
> 
> Sorry, I meant of_iomap(). I think it's only for historic reaons
> that we have both. Probably of_iomap started out on powerpc with
> the same intention as the sparc of_ioremap.
> 
> of_iomap does not require a resource, just an index.

Ah, this sounds like a better solution.  I'll have a play, thanks.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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