diff mbox

[v2,2/4] Documentation: Add APM X-Gene SoC 6.0Gbps SATA PHY driver binding documentation

Message ID 1384905197-3566-3-git-send-email-lho@apm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Loc Ho Nov. 19, 2013, 11:53 p.m. UTC
Documentation: Add APM X-Gene SoC 6.0Gbps SATA PHY driver binding documentation

Document the DTS binding for the X-Gene SoC SATA PHY driver.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Tuan Phan <tphan@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---
 .../devicetree/bindings/ata/apm-xgene-phy.txt      |   61 ++++++++++++++++++++
 1 files changed, 61 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ata/apm-xgene-phy.txt

Comments

Mark Rutland Nov. 20, 2013, 9:43 a.m. UTC | #1
On Tue, Nov 19, 2013 at 11:53:15PM +0000, Loc Ho wrote:
> Documentation: Add APM X-Gene SoC 6.0Gbps SATA PHY driver binding documentation
> 
> Document the DTS binding for the X-Gene SoC SATA PHY driver.
> 
> Signed-off-by: Loc Ho <lho@apm.com>
> Signed-off-by: Tuan Phan <tphan@apm.com>
> Signed-off-by: Suman Tripathi <stripathi@apm.com>
> ---
>  .../devicetree/bindings/ata/apm-xgene-phy.txt      |   61 ++++++++++++++++++++
>  1 files changed, 61 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/ata/apm-xgene-phy.txt
> 
> diff --git a/Documentation/devicetree/bindings/ata/apm-xgene-phy.txt b/Documentation/devicetree/bindings/ata/apm-xgene-phy.txt
> new file mode 100644
> index 0000000..bbae164
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/apm-xgene-phy.txt
> @@ -0,0 +1,61 @@
> +* APM X-Gene 6.0 Gb/s SATA PHY nodes
> +
> +SATA PHY nodes are defined to describe on-chip Serial ATA PHY. Each SATA PHY
> +(pair of PHY) has its own node.
> +
> +Required properties:
> +- compatible		: Shall be "apm,xgene-ahci-phy" or
> +			  "apm,xgene-ahci-phy2". The "apm,xgene-ahci-phy"
> +			  describes an port shared with SGMII Ethernet port.
> +			  The "apm,xgene-ahci-phy2" describes an port not
> +			  shared with SGMII and the PLL located at another
> +			  memory resource region.

Is there not a better name available than "apm,xgene-ahci-phy2"?

> +- reg			: First PHY memory resource
> +			  Second separate PHY PLL clock memory resource if
> +			  type "apm,xgene-ahci-phy2"

Is ths PLL actually part of the PHY, or does it just feed the PHY?

> +- #phy-cells		: Shall be 0
> +
> +Optional properties:
> +- status		: Shall be "ok" if enabled or "na" if disabled. Default
> +			  is "ok".

Do not use "na", use the standard "disabled".

> +- txeyetuning		: Manual control to fine tune the capture of the serial
> +			  bit lines from the automatic calibrated position.
> +			  Two set of 3-tuple setting for Gen1, Gen2, and Gen3.
> +			  Range from 0 to 0x7f. Default is 0xa.

If you have a property for which the name consists of multiple words,
split the words with '-'. 

What does this actually mean?

What units are these values in? 

What effect do those values have?

> +- txeyedirection	: Eye tuning manual control direction. 0 means sample
> +			  data earlier than the nominal sampling point. 1 means
> +			  sample data later than the nominal sampling point.
> +			  Two set of 3-tuple setting for Gen1, Gen2, and Gen3.
> +			  Default is 0x0.

Likewise, use '-'.

s/than/then/ ?

> +- txboostgain		: Frequency boost and DC gain control. Two set of
> +			  3-tuple setting for Gen1, Gen2, and Gen3. Range is
> +			  between 0 to 0x1f. Default is 0x3.

Use '-'.

What units are these values in?

> +- txspeed		: Tx operating speed. Two set of 3-tuple for
> +			  Gen1 (0x1), Gen2 (0x3), and Gen3 (0x7). Default is
> +			  0x7.


Units?

> +
> +NOTE: PHY override parameters are board specific setting.
> +
> +Example:
> +		sataphy0: sataphy@1f210000 {
> +			compatible = "apm,xgene-ahci-phy";
> +			reg = <0x0 0x1f210000 0x0 0x10000>;
> +			#phy-cells = <0>;
> +			status = "na";

Please eradicate all instances of "na".

> +		};
> +
> +		sataphy1: sataphy@1f220000 {
> +			compatible = "apm,xgene-ahci-phy";
> +			reg = <0x0 0x1f220000 0x0 0x10000>;
> +			#phy-cells = <0>;
> +			status = "ok";
> +		};
> +
> +		sataphy2: sataphy@1f230000 {
> +			compatible = "apm,xgene-ahci-phy2";
> +			reg = <0x0 0x1f230000 0x0 0x10000
> +			       0x0 0x1f2d0000 0x0 0x10000 >;

Nit: bracket list entries individually, and don't apply random spacing:

reg = <0x0 0x1f230000 0x0 0x10000>,
      <0x0 0x1f2d0000 0x0 0x10000>;

Thanks,
Mark.
Loc Ho Nov. 20, 2013, 8:07 p.m. UTC | #2
Hi,

>> +- compatible         : Shall be "apm,xgene-ahci-phy" or
>> +                       "apm,xgene-ahci-phy2". The "apm,xgene-ahci-phy"
>> +                       describes an port shared with SGMII Ethernet port.
>> +                       The "apm,xgene-ahci-phy2" describes an port not
>> +                       shared with SGMII and the PLL located at another
>> +                       memory resource region.
>
> Is there not a better name available than "apm,xgene-ahci-phy2"?
[Loc Ho]
I will name it apm,xgene-ahci-phyx.

>> +- reg                        : First PHY memory resource
>> +                       Second separate PHY PLL clock memory resource if
>> +                       type "apm,xgene-ahci-phy2"
>
> Is ths PLL actually part of the PHY, or does it just feed the PHY?
[Loc Ho]
This is always the PLL clock macro in each IP. For the 3rd controller,
we require an extra clock macro and can NOT use the original clock
macro as it is not mux'ed with the SGMII port.

>
>> +- txeyetuning                : Manual control to fine tune the capture of the serial
>> +                       bit lines from the automatic calibrated position.
>> +                       Two set of 3-tuple setting for Gen1, Gen2, and Gen3.
>> +                       Range from 0 to 0x7f. Default is 0xa.
>
> If you have a property for which the name consists of multiple words,
> split the words with '-'.
>
> What does this actually mean?
>
> What units are these values in?
>
> What effect do those values have?
[Loc Ho]
Will provide in next version.

>
>> +- txeyedirection     : Eye tuning manual control direction. 0 means sample
>> +                       data earlier than the nominal sampling point. 1 means
>> +                       sample data later than the nominal sampling point.
>> +                       Two set of 3-tuple setting for Gen1, Gen2, and Gen3.
>> +                       Default is 0x0.
>
> Likewise, use '-'.
>
> s/than/then/ ?
[Loc Ho]
I believe that than is appropriate here a we are doing an comparison.

-Loc
Mark Rutland Nov. 22, 2013, 9:37 a.m. UTC | #3
On Wed, Nov 20, 2013 at 08:07:45PM +0000, Loc Ho wrote:
> Hi,
> 
> >> +- compatible         : Shall be "apm,xgene-ahci-phy" or
> >> +                       "apm,xgene-ahci-phy2". The "apm,xgene-ahci-phy"
> >> +                       describes an port shared with SGMII Ethernet port.
> >> +                       The "apm,xgene-ahci-phy2" describes an port not
> >> +                       shared with SGMII and the PLL located at another
> >> +                       memory resource region.
> >
> > Is there not a better name available than "apm,xgene-ahci-phy2"?
> [Loc Ho]
> I will name it apm,xgene-ahci-phyx.
> 
> >> +- reg                        : First PHY memory resource
> >> +                       Second separate PHY PLL clock memory resource if
> >> +                       type "apm,xgene-ahci-phy2"
> >
> > Is ths PLL actually part of the PHY, or does it just feed the PHY?
> [Loc Ho]
> This is always the PLL clock macro in each IP. For the 3rd controller,
> we require an extra clock macro and can NOT use the original clock
> macro as it is not mux'ed with the SGMII port.

Ok. Is the reference clock into the PLL always on, or might we need to
describe it?

> 
> >
> >> +- txeyetuning                : Manual control to fine tune the capture of the serial
> >> +                       bit lines from the automatic calibrated position.
> >> +                       Two set of 3-tuple setting for Gen1, Gen2, and Gen3.
> >> +                       Range from 0 to 0x7f. Default is 0xa.
> >
> > If you have a property for which the name consists of multiple words,
> > split the words with '-'.
> >
> > What does this actually mean?
> >
> > What units are these values in?
> >
> > What effect do those values have?
> [Loc Ho]
> Will provide in next version.

Cheers.

> 
> >
> >> +- txeyedirection     : Eye tuning manual control direction. 0 means sample
> >> +                       data earlier than the nominal sampling point. 1 means
> >> +                       sample data later than the nominal sampling point.
> >> +                       Two set of 3-tuple setting for Gen1, Gen2, and Gen3.
> >> +                       Default is 0x0.
> >
> > Likewise, use '-'.
> >
> > s/than/then/ ?
> [Loc Ho]
> I believe that than is appropriate here a we are doing an comparison.

Apologies, you are corerct; I completely misread this.

Thanks,
Mark.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/ata/apm-xgene-phy.txt b/Documentation/devicetree/bindings/ata/apm-xgene-phy.txt
new file mode 100644
index 0000000..bbae164
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/apm-xgene-phy.txt
@@ -0,0 +1,61 @@ 
+* APM X-Gene 6.0 Gb/s SATA PHY nodes
+
+SATA PHY nodes are defined to describe on-chip Serial ATA PHY. Each SATA PHY
+(pair of PHY) has its own node.
+
+Required properties:
+- compatible		: Shall be "apm,xgene-ahci-phy" or
+			  "apm,xgene-ahci-phy2". The "apm,xgene-ahci-phy"
+			  describes an port shared with SGMII Ethernet port.
+			  The "apm,xgene-ahci-phy2" describes an port not
+			  shared with SGMII and the PLL located at another
+			  memory resource region.
+- reg			: First PHY memory resource
+			  Second separate PHY PLL clock memory resource if
+			  type "apm,xgene-ahci-phy2"
+- #phy-cells		: Shall be 0
+
+Optional properties:
+- status		: Shall be "ok" if enabled or "na" if disabled. Default
+			  is "ok".
+- txeyetuning		: Manual control to fine tune the capture of the serial
+			  bit lines from the automatic calibrated position.
+			  Two set of 3-tuple setting for Gen1, Gen2, and Gen3.
+			  Range from 0 to 0x7f. Default is 0xa.
+- txeyedirection	: Eye tuning manual control direction. 0 means sample
+			  data earlier than the nominal sampling point. 1 means
+			  sample data later than the nominal sampling point.
+			  Two set of 3-tuple setting for Gen1, Gen2, and Gen3.
+			  Default is 0x0.
+- txboostgain		: Frequency boost and DC gain control. Two set of
+			  3-tuple setting for Gen1, Gen2, and Gen3. Range is
+			  between 0 to 0x1f. Default is 0x3.
+- txspeed		: Tx operating speed. Two set of 3-tuple for
+			  Gen1 (0x1), Gen2 (0x3), and Gen3 (0x7). Default is
+			  0x7.
+
+NOTE: PHY override parameters are board specific setting.
+
+Example:
+		sataphy0: sataphy@1f210000 {
+			compatible = "apm,xgene-ahci-phy";
+			reg = <0x0 0x1f210000 0x0 0x10000>;
+			#phy-cells = <0>;
+			status = "na";
+		};
+
+		sataphy1: sataphy@1f220000 {
+			compatible = "apm,xgene-ahci-phy";
+			reg = <0x0 0x1f220000 0x0 0x10000>;
+			#phy-cells = <0>;
+			status = "ok";
+		};
+
+		sataphy2: sataphy@1f230000 {
+			compatible = "apm,xgene-ahci-phy2";
+			reg = <0x0 0x1f230000 0x0 0x10000
+			       0x0 0x1f2d0000 0x0 0x10000 >;
+			#phy-cells = <0>;
+			status = "ok";
+		};
+