Re: [PATCH v2 5/5] Documentation: Add documentation for APM X-Gene SATA DTS binding

[Arnd Bergmann <arnd@arndb.de>]

On Monday 11 November 2013, Loc Ho wrote:
> Hi Arnd,
> 
> >> ---
> >>  .../devicetree/bindings/ata/apm-xgene.txt          |   84 ++++++++++++++++++++
> >
> > Please Cc the devicetree-discuss mailing list for the binding submission.
> [Loc Ho]
> I did cc on the first version. But this email
> 'devicetree-discuss@lists.ozlabs.org' bounced on me.

It has recently moved to devicetree@vger.kernel.org

> >> +- clocks             : Reference to the clock entry
> >> +- clock-names                : Shall be "eth01clk", "eth23clk", or "eth45clk".
> >
> > This makes no sense. The clock-names property needs to have a fixed
> > string according to the name of the clock input signal of the hardware,
> > not a name of the clock provider.
> [Loc Ho]
> The clock "eth01clk", "eth23clk", and "eth45clk" are the internal
> divider clock. They are not the physical input clock. It sounds like
> we don't need the "clock-names" are all.

Ok.

> 
> >
> >> +- serdes-diff-clk    : Shall be 0 for external, 1 internal differential,
> >> +                       or 2 internal single ended clock. Default is 0.
> >> +- gen-sel            : Shall be 1 (force Gen1), 2 (Force Gen2, or 3 Gen3).
> >> +                       Default is 3.
> >> +- EQA1                       : Serdes EQ parameter for A1 chip. Default is 9.
> >> +- EQ                 : Serdes EQ parameter for non-A1 chip. Default is 2.
> >> +- GENAVG             : Enable averaging Serdes calculation. Default is 0 for
> >> +                       A1 chip and 1 for non-A1 chip.
> >> +- LBA1                       : Serdes loopback buffer for A1 chip. Default is 1;
> >> +- LB                 : Serdes loopback buffer for non-A1 chip. Default is 0;
> >> +- LCA1                       : Serdes loopback enable control for A1 chip. Default
> >> +                       is 1;
> >> +- LC                 : Serdes loopback enable control for non-A1 chip.
> >> +                       Default is 0;
> >> +- CDRA1                      : Serdes SPD select CDR for A1 chip. Default is 5.
> >> +- CDR                        : Serdes SPD select CDR for non-A1 chip. Default is 5.
> >> +- PQA1                       : Serdes PQ for A1 chip. Default is 8.
> >> +- PQ                 : Serdes PQ for non-A1 chip. Default is 0xA.
> >> +- coherent           : Enable coherent (1 = enable, 0 = disable).
> >> +                       Default is 1.
> >
> > This looks like a really bad binding. I would suggest that instead of having
> > individual register values in here, you hardwire the settings in the driver
> > based on the compatible string. It's pretty crazy to put register-level configuration
> > in the DT like this.
> [Loc Ho]
> If I hardwire them in the driver, it will NOT scale across multiple
> board. I guess if I moved it out to the PHY driver, then we can
> discuss in that driver.

Yes, makes sense. If you need the values to change per board, it's probably best to
come up with a somewhat higher-level representation of the same contents. Ideally
we should be able to use the same properties for any SerDes PHY, regarless of
how the register level interface is implemented.

	Arnd