linux-kernel - Re: [PATCH 21/33 v2] clk: ux500: Add Device Tree support for the PRCC Kernel clock

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: <20130619074203.GD31320@laptop>
Date:	Wed, 19 Jun 2013 08:42:03 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Mike Turquette <mturquette@...aro.org>
Cc:	Arnd Bergmann <arnd@...db.de>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linus.walleij@...ricsson.com, srinidhi.kasagar@...ricsson.com,
	Ulf Hansson <ulf.hansson@...aro.org>
Subject: Re: [PATCH 21/33 v2] clk: ux500: Add Device Tree support for the
 PRCC Kernel clock

> Quoting Arnd Bergmann (2013-06-12 07:46:30)
> > On Tuesday 11 June 2013, Lee Jones wrote:
> > > This patch enables clocks to be specified from Device Tree via phandles
> > > to the "prcc-kernel-clock" node.
> > > 
> > > Cc: Mike Turquette <mturquette@...aro.org>
> > > Cc: Ulf Hansson <ulf.hansson@...aro.org>
> > > Signed-off-by: Lee Jones <lee.jones@...aro.org>
> > > 
> > 
> > I don't understand the design of the common clock subsystem here, but is it really
> > necessary to hardcode all the clocks using clk_reg_prcc_kclk() here *and* register
> > a clkdev *and* store the pointer in an array, when you can get all that information
> > from the device tree?
> > 
> > Mike?
> 
> I'm a bit confused by what is going on here too.  There are several
> different ways to handle this.
> 
> 1) Since you have your own clock driver (e.g. not the basic clock types)
> then you could expand the argument list of clk_reg_prcc_kclk to include
> the clkdev dev_id string and toss the call to clk_register_clkdev inside
> of clk_reg_prcc_kclk.

Yes, that's actually a pretty good idea. It has nothing to do with
this patchset, but I will add it to my TODO.

NB: I am away from tomorrow until after Connect, so I will continue
with this after that.

> 2) Move your clock data into DT and teach the driver to use of_clk_get
> to fetch the clk phandle directly instead of using the string-matching
> clkdev mechanisms. Of course both your clock and device bits need to be
> converted to DT first.

I'm sure this is the end-goal, but we still have to support the !DT
case. At least until all of the ATAGs stuff has been completely
removed from platform.

> Can you explain what prcc_kclk[] and friends are doing? Is that a legacy
> clock framework thing that is still hanging around? I'm curious to know
> why your clock driver needs a list of the clocks it registers.

Sure.

1. So when we register a clock, we store a pointer to it in a 'struct
clk *array[]' using known cell identifying read-ins. For peripheral
(pclk) and kernel (kclk) clocks these are peripheral number (the
kernel clocks have these too) and their associated 8bit register
enable BIT(). The PRCMU clocks are read-in to the array only by their
32bit register enable BIT().

2. We then register with of_clk_add_provider() passing the array using
the 'void *data' argument. So:

  clk = clk_reg_prcc_[p|k]clk(blah, blah, blah);
  array[(periph * PRCC_PERIPHS_PER_CLUSTER) + bit] = clk
  of_clk_add_provider(child, ux500_twocell_get, array);

3. In the DTS(I) files we request clocks using their known identifiers
by way of tuplets for the kclks and pclks and by a 1 #cell variant for
the PRCMU clocks. So:

pclk & kclk:
  /*        pclk/kclk       periph  BIT() */
  clocks = <&prcc_[p|k]clk  1       9>;

PRCMU:
  /*        prcmu       BIT()             */
  clocks = <&prcmu_clk  PRCMU_DMACLK>;

The PRCMU can then use the clk framework supplied
of_clk_src_onecell_get() call-back and the pclk and kclks use the 2
#cell variant we provide. They both read into the aforementioned
array[]s we populated earlier in the process to fetch the correct
'struct clk*'.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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/