From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [PATCH v2 5/5] Documentation: Add documentation for APM X-Gene SATA DTS binding Date: Mon, 11 Nov 2013 20:06:44 +0100 Message-ID: <201311112006.44921.arnd@arndb.de> References: <1383980431-6572-1-git-send-email-lho@apm.com> <201311102139.10082.arnd@arndb.de> Mime-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Return-path: Received: from moutng.kundenserver.de ([212.227.17.8]:51600 "EHLO moutng.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754119Ab3KKTG5 (ORCPT ); Mon, 11 Nov 2013 14:06:57 -0500 In-Reply-To: Sender: linux-scsi-owner@vger.kernel.org List-Id: linux-scsi@vger.kernel.org To: Loc Ho Cc: "linux-arm-kernel@lists.infradead.org" , Olof Johansson , tj@kernel.org, linux-scsi@vger.kernel.org, Jon Masters , Suman Tripathi , Tuan Phan 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