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

Arnd Bergmann
2013-11-11T19:06:57Z

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