linux-kernel - Re: [PATCH 1/2] dt: watchdog: Add DT binding documentation for jz47xx watchdog timer

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <9081741.fTg5kD1W54@wuerfel>
Date:	Tue, 27 Jan 2015 23:42:14 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	Zubair Lutfullah Kakakhel <Zubair.Kakakhel@...tec.com>,
	wim@...ana.be, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-watchdog@...r.kernel.org,
	paul@...pouillou.net
Subject: Re: [PATCH 1/2] dt: watchdog: Add DT binding documentation for jz47xx watchdog timer

On Tuesday 27 January 2015 23:36:36 Arnd Bergmann wrote:
> On Tuesday 27 January 2015 14:19:09 Guenter Roeck wrote:
> > On Tue, Jan 27, 2015 at 10:29:49PM +0100, Arnd Bergmann wrote:
> > > On Tuesday 27 January 2015 12:52:29 Guenter Roeck wrote:
> > > > Driver does this (today):
> > > > 
> > > >          drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
> > > > 
> > > > Isn't that the name to use ? Just wondering.
> > > 
> > > Just because the driver uses it at the moment does not mean it's the name
> > > that the IP block uses.
> > > 
> > > clk_get() has the unpleasant property of doing fuzzy matching
> > > on the name that is passed. It first tries to use the string
> > > as the name of the clock input of the device, but if that is
> > > not there, it falls back to looking for a global clk with a con_id.
> > > 
> > > In DT, we only support the first kind, but if a driver currently
> > > uses the second, you get the wrong name.
> > > 
> > > Looking at arch/mips/jz4740/clock.c now, this seems to be exactly
> > > what is going on here: there is no clkdev_add call to associate
> > > the device clocks, so it can only match a global clock entry. 
> > > 
> > Me confused :-(.
> > 
> > Does that mean the driver needs to be fixed, that the DT property
> > needs to change (to what ?), or both ?
> 
> Both.
> 
> The jz47xx clock driver should register a clkdev lookup table with
> proper clock input names for each clock that is referenced by a
> device, and then the drivers can use the right names.
> 
> In a lot of cases, the best name for a clock is no name so you
> just use an anonymous clock like
> 
>         clk = clk_get(dev, NULL);
> 
> but this still requires a clock lookup table.

To illustrate why the current approach is wrong, think of a driver
that handles a device that can be used on two different SoCs.

The name used by the clock provider is SoC specific, so the driver
would need to know which SoC it's being used on in order to pass
the right clock signal name. clkdev is meant to solve this by providing
a lookup between the device/clock-input pair and the actual clock.

Apparently jz4740 does not share any devices with another SoC at
the moment, so this has not been a problem, but if jz4780 has
a slightly different clock tree, it's already broken.

	Arnd
--
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/