| Message ID | 1383980431-6572-6-git-send-email-lho@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Saturday 09 November 2013, Loc Ho wrote: > Documentation: Add documentation for APM X-Gene SATA DTS binding > > 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.txt | 84 ++++++++++++++++++++ Please Cc the devicetree-discuss mailing list for the binding submission. > +SATA nodes are defined to describe on-chip Serial ATA controllers. > +Each SATA controller (pair of ports) have its own node. > + > +Required properties: > +- compatible : Shall be "apm,xgene-ahci" > +- reg : First memory resource shall be the AHCI memory resource > + Second memory resource shall be the Serdes memory resource > + Third memory resource shall be the optional Serdes > + memory resource if mux'ed with another IP > +- interrupt-parent : Interrupt controller > +- interrupts : Interrupt mapping for SATA IRQ > +- #clock-cells : Shall be value of 1 Why is there a #clock-cells entry here? > +- 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. > +- 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. Further, you should probably use the generic PHY binding to create a separate driver for the serdes PHY, Arnd
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. >> +- interrupt-parent : Interrupt controller >> +- interrupts : Interrupt mapping for SATA IRQ >> +- #clock-cells : Shall be value of 1 > > Why is there a #clock-cells entry here? [Loc Ho] Okay... Let me see if I can get rip of this. > >> +- 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. > >> +- 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. > > Further, you should probably use the generic PHY binding to create a separate driver > for the serdes PHY, > [Loc Ho] I will look into this. -Loc
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
diff --git a/Documentation/devicetree/bindings/ata/apm-xgene.txt b/Documentation/devicetree/bindings/ata/apm-xgene.txt new file mode 100644 index 0000000..cd52864 --- /dev/null +++ b/Documentation/devicetree/bindings/ata/apm-xgene.txt @@ -0,0 +1,84 @@ +* APM X-Gene 6.0 Gb/s SATA nodes + +SATA nodes are defined to describe on-chip Serial ATA controllers. +Each SATA controller (pair of ports) have its own node. + +Required properties: +- compatible : Shall be "apm,xgene-ahci" +- reg : First memory resource shall be the AHCI memory resource + Second memory resource shall be the Serdes memory resource + Third memory resource shall be the optional Serdes + memory resource if mux'ed with another IP +- interrupt-parent : Interrupt controller +- interrupts : Interrupt mapping for SATA IRQ +- #clock-cells : Shall be value of 1 +- clocks : Reference to the clock entry +- clock-names : Shall be "eth01clk", "eth23clk", or "eth45clk". + +Optional properties: +- status : Shall be "ok" if enabled or "na" if disabled. Default + is "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. + +Example: + sata0: sata@1a000000 { + compatible = "apm,xgene-ahci"; + reg = <0x0 0x1a000000 0x0 0x100000 + 0x0 0x1f210000 0x0 0x10000>; + interrupt-parent = <&gic>; + interrupts = <0x0 0x86 0x4>; + #clock-cells = <1>; + clocks = <ð01clk 0>; + clock-names = "eth01clk"; + status = "na"; + serdes-diff-clk = <0>; + gen-sel = <3>; + }; + + sata1: sata@1a400000 { + compatible = "apm,xgene-ahci"; + reg = <0x0 0x1a400000 0x0 0x100000 + 0x0 0x1f220000 0x0 0x10000>; + interrupt-parent = <&gic>; + interrupts = <0x0 0x87 0x4>; + #clock-cells = <1>; + clocks = <ð23clk 0>; + clock-names = "eth23clk"; + status = "na"; + serdes-diff-clk = <0>; + gen-sel = <3>; + }; + + sata2: sata@1a800000 { + compatible = "apm,xgene-ahci"; + reg = <0x0 0x1a800000 0x0 0x100000 + 0x0 0x1f230000 0x0 0x10000 + 0x0 0x1f2d0000 0x0 0x10000 >; + interrupt-parent = <&gic>; + interrupts = <0x0 0x88 0x4>; + #clock-cells = <1>; + clocks = <&sata45clk 0>; + clock-names = "sata45clk"; + status = "ok"; + serdes-diff-clk = <0>; + gen-sel = <3>; + };