Re: [PATCH v4 2/4] Documentation: Add APM X-Gene SoC 15Gbps Multi-purpose PHY driver binding documentation

[Douglas Gilbert <dgilbert@interlog.com>]

On 13-12-12 02:27 PM, Arnd Bergmann wrote:
> On Thursday 12 December 2013, Loc Ho wrote:
>> +- reg                  : First PHY memory resource is the SDS PHY access
>> +                         resource.
>> +                         Second PHY memory resoruce is the clock and reset
>> +                         resources.
>> +                         Third PHY memory resource is the SDS PHY access
>> +                         resource outside of the IP if it is type
>> +                         "apm,xgene-phy-ext".
>
> Why do the "clock and reset" resources not use a clock driver and a reset
> driver?
>
> I would expect these to get replaced with
>
> 	clocks		: Reference to external clock input
> 	resets		: Reference to reset controller input
>
>> +Optional properties:
>> +- status		: Shall be "ok" if enabled or "disabled" if disabled.
>> +			  Default is "ok".
>> +- apm,tx-eye-tuning	: 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 in unit of one bit period.
>> +			  Default is 0xa.
>
> What does gen1, gen2 and gen3 refer to? Is this PCIe, SATA or serdes generations
> or all of them?
>
> Why are there two sets?
>
> Will this have to change if you add PCIe support?
>
> I would suggest using decimal notation here instead of hexadecimal since you
> are dealing with numbers couting things. Same for the others.
>
>> +- apm,tx-eye-direction	: 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.
>> +
>> +- apm,tx-boost-gain	: Frequency boost AC (LSB 3-bit) and DC (2-bit)
>> +			  gain control. Two set of 3-tuple setting for Gen1,
>> +			  Gen2, and Gen3. Range is between 0 to 0x1f in unit
>> +			  of dB. Default is 0x3.
>> +
>> +- apm,tx-amplitude	: Amplitude control. Two set of 3-tuple setting for
>> +			  Gen1, Gen2, and Gen3. Range is between 0 to 0xf in
>> +			  unit of 13.3mV. Default is 0xf.
>
> Units of 13.3mV don't seem to be useful as a generic measurement. I'd
> recommend using milivolts or microvolts.
>
>> +- apm,tx-pre-cursor1	: 1st pre-cursor emphasis taps control. Two set of
>> +			  3-tuple setting for Gen1, Gen2, and Gen3. Range is
>> +			  between 0 to 0xf in unit of 18.2mV. Default is 0x0.
>> +- apm,tx-pre-cursor2	: 2st pre-cursor emphasis taps control. Two set of
>> +			  3-tuple setting for Gen1, Gen2, and Gen3. Range is
>> +			  between 0 to 0x7 in unit of 18.2mV. Default is 0x0.
>> +- apm,tx-post-cursor	: Post-cursor emphasis taps control. Two set of
>> +			  3-tuple setting for Gen1, Gen2, and Gen3. Range is
>> +			  between 0 to 0x1f in unit of 18.2mV. Default is 0xf.
>
> Same here.
>
>> +- apm,tx-speed		: Tx operating speed. One set of 3-tuple for
>> +			  Gen1 (0x1), Gen2 (0x3), and Gen3 (0x7). Default is
>> +			  0x7.
>
> I'm completely confused by this description. Can you rephrase this?
> It sounds like the only possible values are <1 3 7> for this property.

Most likely Gen1, Gen2 and Gen3 are SATA-speak corresponding to SAS's
G1, G2 and G3:

G1   Gen1     1.5 Gbps
G2   Gen2     3 Gbps
G3   Gen3     6 Gbps
G4   -        12 Gbps
G5   -        24 Gbps

And the "7" corresponding to Gen3 is indicating backward compatibility
with Gen2 and Gen1. The SAS-3 draft only requires backward compatibility
two generations. Thus you can buy a SAS 12 Gbps HBA today that will
not support the original SATA 1.5 Gbps class of disks. The corresponding
value would be 0xe (rather than 0xf) using the tx-speed convention above.


My explanation is a bit long winded to put in a device-tree bindings
file. "RTFM: SATA drafts." should suffice.


BTW Compared to some device-tree binding explanations I have had
to wade through, the above looks pretty good.

Doug Gilbert