Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759871AbcDESpL (ORCPT ); Tue, 5 Apr 2016 14:45:11 -0400 Received: from comal.ext.ti.com ([198.47.26.152]:57642 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759722AbcDESpJ (ORCPT ); Tue, 5 Apr 2016 14:45:09 -0400 Subject: Re: [PATCH v2 1/2] Documentation: dt: reset: Add syscon reset binding To: Rob Herring , Philipp Zabel References: <1457646365-1723-1-git-send-email-afd@ti.com> <1457646365-1723-2-git-send-email-afd@ti.com> <20160318163918.GA13101@rob-hp-laptop> <56EC349D.3040100@ti.com> CC: "Andrew F. Davis" , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , , From: Suman Anna Message-ID: <5704079D.7080203@ti.com> Date: Tue, 5 Apr 2016 13:44:45 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <56EC349D.3040100@ti.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5240 Lines: 124 On 03/18/2016 12:02 PM, Suman Anna wrote: > Hi Rob, > > On 03/18/2016 11:39 AM, Rob Herring wrote: >> On Thu, Mar 10, 2016 at 03:46:04PM -0600, Andrew F. Davis wrote: >>> Add syscon reset controller binding. This will hook to the reset >>> framework and use syscon/regmap to set reset bits. This allows >>> reset control of individual SoC subsytems and devices with >>> memory-mapped reset registers in a common register memory >>> space. >>> >>> Signed-off-by: Andrew F. Davis >>> [s-anna@ti.com: revise the binding format] >>> Signed-off-by: Suman Anna >>> --- >>> .../devicetree/bindings/reset/syscon-reset.txt | 114 +++++++++++++++++++++ >>> include/dt-bindings/reset/syscon.h | 23 +++++ >>> 2 files changed, 137 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt >>> create mode 100644 include/dt-bindings/reset/syscon.h >>> >>> diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt >>> new file mode 100644 >>> index 0000000..02bc9e3 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt >>> @@ -0,0 +1,114 @@ >>> +SysCon Reset Controller >>> +======================= >>> + >>> +Almost all SoCs have hardware modules that require reset control in addition >>> +to clock and power control for their functionality. The reset control is >>> +typically provided by means of memory-mapped I/O registers. These registers are >>> +sometimes a part of a larger register space region implementing various >>> +functionalities. This register range is best represented as a syscon node to >>> +allow multiple entities to access their relevant registers in the common >>> +register space. >>> + >>> +A SysCon Reset Controller node defines a device that uses a syscon node >>> +and provides reset management functionality for various hardware modules >>> +present on the SoC. >>> + >>> +SysCon Reset Controller Node >>> +============================ >>> +Each of the reset provider/controller nodes should be a child of a syscon >>> +node and have the following properties. >>> + >>> +Required properties: >>> +-------------------- >>> + - compatible : Should be "syscon-reset" >>> + - #reset-cells : Should be 1. Please see the reset consumer node below >>> + for usage details >>> + - #address-cells : Should be 1 >>> + - #size-cells : Should be 0 >>> + >>> +SysCon Reset Child Node >>> +============================ >>> +Each reset provider/controller node should have a child node for each reset >>> +it would like to expose to consumers. >> >> A node per register bit... Now I'm only more convinced this is too low >> level. > > Thanks for the review/comments. So, what's your recommendation here - > add the reset info to driver match data and use SoC-specific > compatibles? That will also work, we just didn't go that route as it > appeared to be adding hardware data to a driver. > >>> + >>> +Required properties: >>> +-------------------- >>> + - reg : This reset's number, this will be used to reference >>> + this reset by consumers of this reset >> >> Now you have a made up index unrelated to anything in the h/w. Sometimes >> that's unavoidable, but your prior version did. > > We have made this change to avoid adding the reset data in the consumer > nodes as was done in v1, and provide it in the controller node itself. > We still need a way for consumers to match a specific reset. This is > unavoidable if the controller were to encode all the reset data (even if > we go with coding up the resets in the driver using SoC-specific > compatibles, and use a dt-include header file to mark the indices). Hi Rob, Philipp, Any more comments on this series? If you don't have any, we will repost with some minor cleanups/updates against 4.6-rc1 for the next merge window. Or do you feel that the generic driver is still not the way to go and restrict this to a keystone specific driver? regards Suman > > regards > Suman > >> >>> + - reset-control : Contains the reset control register information >>> + Should contain 3 cells defined as: >>> + Cell #1 : register offset of the reset >>> + control/status register from the syscon >>> + register base >>> + Cell #2 : bit shift value for the reset in the >>> + respective reset control/status register >>> + Cell #3 : polarity of the reset bit. Should be 1 or >>> + RESET_ASSERT_SET for resets that are >>> + asserted when the bit is set, 0 or >>> + RESET_ASSERT_CLEAR for resets that are >>> + asserted when the bit is cleared >>> + >>> +Optional properties: >>> +-------------------- >>> + - reset-status : Contains the reset status register information. The >>> + contents of this property are the equivalent to >>> + reset-control as defined above. If this property is >>> + not present and the toggle flag is not set, the >>> + reset register is assumed to be the same as the >>> + control register >>> + - toggle : Mark this reset as a toggle only reset, this is used >>> + when no status register is available. >> >