linux-kernel - Re: [PATCH v2] ARM: ls1021a: add gating clocks to IP blocks.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20140925082940.GF6405@dragon>
Date:	Thu, 25 Sep 2014 16:29:41 +0800
From:	Shawn Guo <shawn.guo@...escale.com>
To:	Xiubo Li-B47053 <Li.Xiubo@...escale.com>
CC:	Uwe Kleine-K?nig <u.kleine-koenig@...gutronix.de>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"pawel.moll@....com" <pawel.moll@....com>,
	"mark.rutland@....com" <mark.rutland@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	"galak@...eaurora.org" <galak@...eaurora.org>,
	"kernel@...gutronix.de" <kernel@...gutronix.de>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2] ARM: ls1021a: add gating clocks to IP blocks.

On Mon, Sep 22, 2014 at 03:55:14PM +0800, Xiubo Li-B47053 wrote:
> > > +static void __init ls1021a_clocks_init(struct device_node *np)
> > > +{
> > > +	void __iomem *dcfg_base;
> > > +
> > > +#define DCFG_CCSR_DEVDISR1	(dcfg_base + 0x70)
> > > +#define DCFG_CCSR_DEVDISR2	(dcfg_base + 0x74)
> > > +#define DCFG_CCSR_DEVDISR3	(dcfg_base + 0x78)
> > > +#define DCFG_CCSR_DEVDISR4	(dcfg_base + 0x7c)
> > > +#define DCFG_CCSR_DEVDISR5	(dcfg_base + 0x80)
> > > +
> > > +	dcfg_base = of_iomap(np, 0);
> > > +
> > > +	BUG_ON(!dcfg_base);
> > Is this worth a BUG? 
> Yes, I do think so.
> 
> There is one story about my platform:
> We are using the imx2_wdt watchdog device, which cannot stop or suspend
> once it has started.
> And our platform will also support the Power Management, if the gating
> Clock initialized failed, so when the system enters sleep mode, we cannot
> Stop the imx2_wdt IP block, so it will reset the board after 180 seconds at
> most.
> 
> So using this gating clock, the watchdog IP block's clock could be disabled
> When the system enter sleep mode.
> 
> So I think BUG_ON here is make sense.
>  
> Doesn't it ?
> 
> > Or is it enough to do
> > 	if (!dcfg_base) {
> > 		pr_warn("failed to map fsl,ls1021a-gate device\n");
> > 		return
> > 	}
> > 
> > > +
> > > +	clk[LS1021A_CLK_PBL_EN] = ls1021a_clk_gate("pbl_en", "dummy",
> > > +						DCFG_CCSR_DEVDISR1, 0, true);
> > If the machine's device tree contains two (or more) nodes that are
> > compatible to "fsl,ls1021a-gate", you overwrite your static clk array. Is
> > it worth to add another check here?:
> > 
> On LS1021A SoC, I can make sure that there will only be one gate node. But for
> More compatibly using one dynamic clk array will be better.

I do not think it's really necessary to use dynamic allocation.  Making
dcfg_base a static variable and check if it's null at the beginning of
the function is probably enough.

Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/