From patchwork Wed Apr 29 12:57:04 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Rutland X-Patchwork-Id: 6295821 Return-Path: X-Original-To: patchwork-linux-pm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 0A18D9F1C2 for ; Wed, 29 Apr 2015 12:57:20 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D9328201B4 for ; Wed, 29 Apr 2015 12:57:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 22C77201C8 for ; Wed, 29 Apr 2015 12:57:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1422884AbbD2M5P (ORCPT ); Wed, 29 Apr 2015 08:57:15 -0400 Received: from foss.arm.com ([217.140.101.70]:33049 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1422821AbbD2M5N (ORCPT ); Wed, 29 Apr 2015 08:57:13 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C021829; Wed, 29 Apr 2015 05:56:47 -0700 (PDT) Received: from collaborate-mta1.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTP id 1F6053F21A; Wed, 29 Apr 2015 05:57:13 -0700 (PDT) Received: from leverpostej (leverpostej.cambridge.arm.com [10.1.205.151]) by collaborate-mta1.arm.com (Postfix) with ESMTPS id 36E2013F885; Wed, 29 Apr 2015 07:57:11 -0500 (CDT) Date: Wed, 29 Apr 2015 13:57:04 +0100 From: Mark Rutland To: Geert Uytterhoeven Cc: Geert Uytterhoeven , Rob Herring , Pawel Moll , Ian Campbell , Kumar Gala , Marc Zyngier , "devicetree@vger.kernel.org" , "linux-pm@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-sh@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Magnus Damm Subject: Re: [PATCH] ARM: gic: Document Power and Clock Domain optional properties Message-ID: <20150429125703.GB11757@leverpostej> References: <1430146811-29862-1-git-send-email-geert+renesas@glider.be> <20150427155427.GB16191@leverpostej> <20150427171502.GD16191@leverpostej> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Geert, > >> >> To preserve DT stability, we would like to add these properties to the > >> >> affected shmobile dtsi files. > >> > > >> > ... which means that they could be wrong, and will get in the way of > >> > stability rather than aiding it. > >> > >> We do know the GIC is part of the power domain, and has a controllable > >> clock (on the affected SoCs). > > > > ... my concern is that the data we place into the DT will be untested > > given that we don't have software relying on it. If said data is not > > correct, it is harmful to have, especially for such fundamental > > properties. > > Your statement challenges the viability of Stable DT Requirements, as we > can thus never write a DTS until the full software implementation has been > completed ;-) I appreciate this is difficult, but I disagree that it's impossible ;) If you don't want to do clock management currently, don't describe the clock controller, have some FW/loader pre-program the clocks, and list fixed-clocks in the DTB. This DTB should continue to work, but a new kernel alone won't give you fancy clock management. This is what we expect in terms of stable DTBs. When you add clock controller support, you need a different DT to describe the clock controller anyway. You can have it nuke the clock configuration at boot time as a test that everything you need is described. > >> > I'm also concerned that the carving up of clock inputs, power domains, > >> > and other physical details is implementation-specific. I imagine that > >> > pretty much every user that will care about this is using GIC-400, so > >> > could we make this specific to GIC-400? > >> > >> I have no idea which GIC version is being used. > > > > This is unfortunate. > > > >> This is for Renesas R-Mobile APE6 (r8a73a4) and various R-Car Gen2. > >> According to the DTS, they're compatible with "arm,cortex-a15-gic" and > >> "arm,cortex-a7-gic", and work with that value. > > > > Who put the DT together in the first place? > > Magnus (added to CC). > > > If it's a multi-cluster SoC then we know that we're not using any > > built-in distributor... > > R-Mobile APE6 has 2 clusters (4xCA15 and 4xCA7). > R-Car Gen2 has 1 or 2 clusters (2/4xCA15 and/or 2/4xCA7). It looks like we should be able to read the GICD_IIDR to figure out what imlpementation is used. Could you see what GICD_IIDR reports on those platforms? There's a patch at the end of the email to do so. [...] > >> >> +- clocks : A phandle and clock specifier as defined by bindings of > >> >> + the clock controller specified by phandle, used when the GIC > >> >> + is part of a Clock Domain. > >> > > >> > Depending on implementation, a GIC could require multiple clocks, and > >> > their names would be implementation-specific (that said, GIC-400 has a > >> > single "CLK" input). > >> > >> Documentation/devicetree/bindings/arm/gic.txt doesn't mention any clocks > >> for GIC-400? > > > > I'm going by the TRM. There are other items not in the bindign that are > > implementation-specific. > > > >> > Assuming that you're using GIC-400, could we use clock-names to make > >> > that explicit? > >> > >> You can. But you have to be careful to avoid conflicts, as the controller > >> for the clock domain needs to know which clock to use. > > > > Could you elborate on that? I don't see why a node's clock-names > > property should be relevant to the clock controller. > > If there are multiple clocks listed, the clock controller needs to know which > clocks is to be used for power management. > > If we standardize on a name for the clock to be used for power management > (a platform property), that could conflict with the naming in the binding > for the device. > Some device bindings don't mandate clock-names as only a single clock > is used. > Other device bindings do mandate clock-names. You mentioned "CLK" for > GIC-400, others use e.g. "fck", or "ick". The clock-names should be device-binding specific. Nothing else should be there. If you want to do fancy maangement of device clocks, the device drivers should register with the appropriate frameworks to do so, and nothing else needs to inspect the clock-names for the device itself. > >> On Renesas SoCs, we always used the first clock for power control. > >> For the future, we plan to use "fck" if it exists. > > > > That's irrelevant from the PoV of the GIC; per the GIC architecture you > > can't assume anything about the set of input clocks, and GIC-400 happens > > to have a single clock input called "CLK". It's not IP specific to > > Renesas. > > > >> Yes, this can be complicated, as ideally it needs synchronization between > >> device DT bindings and platform (clock/power domain) DT bindings. > >> > >> Please also note that any device (hardware IP core) can be reused on an > >> SoC that supports clock and/or power domains, and suddenly starts to rely > >> on power-domains and clocks properties. > > > > Sorry, I don't follow this last part. > > Devices may describe zero, one, or more clocks in the DT bindings. Usually > these clocks are functional, and they are described because the driver needs > to control them, and/or needs knowledge about them. > > As all (current) logic is synchronous, there's always a clock that drives the > logic of the device. Gating this clock can be used for power management. > This clock may or may not be the one described in the bindings (obviously > it isn't if the binding doesn't describe any clock, but there can be other > cases). > > Any IP core for a device may be reused. On some SoCs, the logic may be > driven by a fixed, non-gateable clock. On others the logic may be driven by > a gateable clock to save power. Whether or not there is a gateable clock > is a property of the platform (SoC), not of the IP core. I agree that the provider of any clock (and it's feature such as gateability) is not device specific. What I am saying is that the set of input lines on an IP block (to which clocks may be connected) are a property of that IP block. As mentioned above, GIC-400 always has a clock input line called "CLK", though this could be wired to arbitrary clock providers, which might call their output line arbitrary names (e.g. "GIC_CLK"). If bindings are missing functional clocks, those can be added to the bindings, and we can warn when they are not present. These clocks are all specific to the particular IP blocks however, so applying an SoC-specific naming policy to these does not make sense. Thanks, Mark. ---->8---- From 8bd8a5fe42d25b45515ef3152ff921d2180a8120 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Wed, 29 Apr 2015 13:54:03 +0100 Subject: [PATCH] irqchip: gic: report GICD_IIDR The GICD_IIDR register provides information about the implementer and revision of the GIC distributor. In some cases this information is useful for debugging, so log this when probing the GIC. Signed-off-by: Mark Rutland --- drivers/irqchip/irq-gic.c | 4 ++++ include/linux/irqchip/arm-gic.h | 1 + 2 files changed, 5 insertions(+) diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 7b315e3..02c8bb4 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -959,6 +959,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, irq_hw_number_t hwirq_base; struct gic_chip_data *gic; int gic_irqs, irq_base, i; + unsigned long iidr; BUG_ON(gic_nr >= MAX_GIC_NR); @@ -996,6 +997,9 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, gic_set_base_accessor(gic, gic_get_common_base); } + iidr = readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IIDR); + pr_info("GICD_IIDR reports 0x%08lx\n", (unsigned long)iidr); + /* * Initialize the CPU interface map to all CPUs. * It will be refined as each CPU probes its ID. diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h index 36ec4ae..c29df66 100644 --- a/include/linux/irqchip/arm-gic.h +++ b/include/linux/irqchip/arm-gic.h @@ -29,6 +29,7 @@ #define GIC_DIST_CTRL 0x000 #define GIC_DIST_CTR 0x004 +#define GIC_DIST_IIDR 0x008 #define GIC_DIST_IGROUP 0x080 #define GIC_DIST_ENABLE_SET 0x100 #define GIC_DIST_ENABLE_CLEAR 0x180