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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Mon, 11 Nov 2013 20:06:44 +0100 Subject: [PATCH v2 5/5] Documentation: Add documentation for APM X-Gene SATA DTS binding In-Reply-To: References: <1383980431-6572-1-git-send-email-lho@apm.com> <201311102139.10082.arnd@arndb.de> Message-ID: <201311112006.44921.arnd@arndb.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 at lists.ozlabs.org' bounced on me. It has recently moved to devicetree at 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