mbox

Convert i.MX architecture to generic clock framework

Message ID 1334065553-7565-1-git-send-email-s.hauer@pengutronix.de
State New
Headers show

Pull-request

git://git.pengutronix.de/git/imx/linux-2.6.git work/v3.4-rc2-imx-clk

Message

Sascha Hauer April 10, 2012, 1:45 p.m. UTC
Hi All,

The following series is the first series converting the complete
i.MX architecture to the generic clock framework. I am currently
waiting for Mike posting his cleanup series to the clock framework
which will cause some adjustments to this series. However, this
series will only need some changes to the interface between i.MX
and the clock framework. The association between the devices and
the clocks (clk_lookup) and also the clocks themselves will stay
unchanged. Due to the huge amount of changes there *will* be
regressions, so now is the time to check if this works with your
favourite devices and boards. I'll happily integrate fixup patches
into this series.

As mentioned before my plan is to put this into the arm-soc staging
area soon (hopefully when Mikes series is posted and acked)

Thanks to Shawn for porting over the i.MX6.

Sascha

The following changes since commit b41c67c587c98eb2efb2a79d4b8122b04b519d4a:

  clkdev: Implement managed clk_get() (2012-04-10 09:35:09 +0200)

are available in the git repository at:

  git://git.pengutronix.de/git/imx/linux-2.6.git work/v3.4-rc2-imx-clk

for you to fetch changes up to 3134a067ba4a66921acc8461165634abbae42e22:

  ARM i.MX: Remove now unused struct clk argument from mxc_timer_init (2012-04-10 09:35:54 +0200)

----------------------------------------------------------------
Sascha Hauer (35):
      clkdev: add clkname to struct clk_lookup
      clk: add a fixed factor clock
      dmaengine i.MX SDMA: do not depend on grouped clocks
      spi i.MX: do not depend on grouped clocks
      video imxfb: do not depend on grouped clocks
      net fec: do not depend on grouped clocks
      mmc mxcmmc: do not depend on grouped clocks
      mmc sdhc i.MX: do not depend on grouped clocks
      serial i.MX: do not depend on grouped clocks
      mtd mxc_nand: prepare/unprepare clock
      USB ehci mxc: prepare/unprepare clock
      w1 i.MX: prepare/unprepare clock
      watchdog imx2: prepare clk before enabling it
      media mx3 camera: prepare clk before enabling it
      dmaengine i.MX ipu: clk_prepare/unprepare clock
      ARM i.MX5: prepare gpc_dvfs_clk
      ARM i.MX: prepare for common clock framework
      ARM i.MX timer: request correct clock
      ARM i.MX: Add common clock support for pllv1
      ARM i.MX: Add common clock support for pllv2
      ARM i.MX: Add common clock support for 2bit gate
      ARM i.MX3: Make ccm base address a variable
      ARM i.MX25: implement clocks using common clock framework
      ARM i.MX1: implement clocks using common clock framework
      ARM i.MX21: implement clocks using common clock framework
      ARM i.MX27: implement clocks using common clock framework
      ARM i.MX31: implement clocks using common clock framework
      ARM i.MX5: implement clocks using common clock framework
      ARM i.MX35: implement clocks using common clock framework
      ARM i.MX: remove now unused old clock support
      ARM i.MX pllv1: move mxc_decode_pll to its only user
      ARM i.MX: remove unused legacy clock support
      USB gadget i.MX: fix clock handling
      USB ehci i.MX: Fix clock handling
      ARM i.MX: Remove now unused struct clk argument from mxc_timer_init

Shawn Guo (5):
      clk: declare clk_ops of basic clks in clk-provider.h
      ARM: imx: add common clock support for pllv3
      ARM: imx: add common clock support for pfd
      ARM: imx: add common clock support for clk busy
      ARM: imx6: implement clocks using common clock framework

 arch/arm/mach-imx/Kconfig               |    8 +
 arch/arm/mach-imx/Makefile              |   19 +-
 arch/arm/mach-imx/clk-busy.c            |  167 +++
 arch/arm/mach-imx/clk-gate2.c           |  125 ++
 arch/arm/mach-imx/clk-imx1.c            |  108 ++
 arch/arm/mach-imx/clk-imx21.c           |  173 +++
 arch/arm/mach-imx/clk-imx25.c           |  232 ++++
 arch/arm/mach-imx/clk-imx27.c           |  254 ++++
 arch/arm/mach-imx/clk-imx31.c           |  167 +++
 arch/arm/mach-imx/clk-imx35.c           |  258 ++++
 arch/arm/mach-imx/clk-imx51-imx53.c     |  412 ++++++
 arch/arm/mach-imx/clk-imx6q.c           |  407 ++++++
 arch/arm/mach-imx/clk-pfd.c             |  138 ++
 arch/arm/mach-imx/clk-pllv1.c           |  104 ++
 arch/arm/mach-imx/clk-pllv2.c           |  243 ++++
 arch/arm/mach-imx/clk-pllv3.c           |  408 ++++++
 arch/arm/mach-imx/clk.h                 |   83 ++
 arch/arm/mach-imx/clock-imx1.c          |  636 ----------
 arch/arm/mach-imx/clock-imx21.c         | 1239 ------------------
 arch/arm/mach-imx/clock-imx25.c         |  346 -----
 arch/arm/mach-imx/clock-imx27.c         |  785 ------------
 arch/arm/mach-imx/clock-imx31.c         |  630 ---------
 arch/arm/mach-imx/clock-imx35.c         |  536 --------
 arch/arm/mach-imx/clock-imx6q.c         | 2111 -------------------------------
 arch/arm/mach-imx/clock-mx51-mx53.c     | 1675 ------------------------
 arch/arm/mach-imx/crmregs-imx3.h        |   79 +-
 arch/arm/mach-imx/mm-imx3.c             |    6 +
 arch/arm/mach-imx/mm-imx5.c             |    1 +
 arch/arm/mach-imx/pm-imx3.c             |    4 +-
 arch/arm/plat-mxc/clock.c               |  228 +---
 arch/arm/plat-mxc/include/mach/clock.h  |   43 +-
 arch/arm/plat-mxc/include/mach/common.h |    2 +-
 arch/arm/plat-mxc/time.c                |   16 +-
 drivers/clk/Makefile                    |    2 +-
 drivers/clk/clk-fixed-factor.c          |   97 ++
 drivers/clk/clkdev.c                    |    8 +
 drivers/dma/imx-sdma.c                  |   40 +-
 drivers/dma/ipu/ipu_idmac.c             |    6 +-
 drivers/media/video/mx3_camera.c        |    4 +-
 drivers/mmc/host/mxcmmc.c               |   39 +-
 drivers/mmc/host/sdhci-esdhc-imx.c      |   42 +-
 drivers/mtd/nand/mxc_nand.c             |    6 +-
 drivers/net/ethernet/freescale/fec.c    |   35 +-
 drivers/spi/spi-imx.c                   |   30 +-
 drivers/tty/serial/imx.c                |   38 +-
 drivers/usb/gadget/fsl_mxc_udc.c        |   74 +-
 drivers/usb/host/ehci-mxc.c             |   58 +-
 drivers/video/imxfb.c                   |   50 +-
 drivers/w1/masters/mxc_w1.c             |    4 +-
 drivers/watchdog/imx2_wdt.c             |    2 +-
 include/linux/clk-private.h             |    8 -
 include/linux/clk-provider.h            |   12 +
 include/linux/clkdev.h                  |    3 +
 53 files changed, 3736 insertions(+), 8465 deletions(-)
 create mode 100644 arch/arm/mach-imx/clk-busy.c
 create mode 100644 arch/arm/mach-imx/clk-gate2.c
 create mode 100644 arch/arm/mach-imx/clk-imx1.c
 create mode 100644 arch/arm/mach-imx/clk-imx21.c
 create mode 100644 arch/arm/mach-imx/clk-imx25.c
 create mode 100644 arch/arm/mach-imx/clk-imx27.c
 create mode 100644 arch/arm/mach-imx/clk-imx31.c
 create mode 100644 arch/arm/mach-imx/clk-imx35.c
 create mode 100644 arch/arm/mach-imx/clk-imx51-imx53.c
 create mode 100644 arch/arm/mach-imx/clk-imx6q.c
 create mode 100644 arch/arm/mach-imx/clk-pfd.c
 create mode 100644 arch/arm/mach-imx/clk-pllv1.c
 create mode 100644 arch/arm/mach-imx/clk-pllv2.c
 create mode 100644 arch/arm/mach-imx/clk-pllv3.c
 create mode 100644 arch/arm/mach-imx/clk.h
 delete mode 100644 arch/arm/mach-imx/clock-imx1.c
 delete mode 100644 arch/arm/mach-imx/clock-imx21.c
 delete mode 100644 arch/arm/mach-imx/clock-imx25.c
 delete mode 100644 arch/arm/mach-imx/clock-imx27.c
 delete mode 100644 arch/arm/mach-imx/clock-imx31.c
 delete mode 100644 arch/arm/mach-imx/clock-imx35.c
 delete mode 100644 arch/arm/mach-imx/clock-imx6q.c
 delete mode 100644 arch/arm/mach-imx/clock-mx51-mx53.c
 create mode 100644 drivers/clk/clk-fixed-factor.c

Comments

Sascha Hauer April 10, 2012, 1:45 p.m. UTC | #1
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-imx/clock-imx1.c      |  636 -----------
 arch/arm/mach-imx/clock-imx21.c     | 1239 --------------------
 arch/arm/mach-imx/clock-imx25.c     |  346 ------
 arch/arm/mach-imx/clock-imx27.c     |  785 -------------
 arch/arm/mach-imx/clock-imx31.c     |  630 -----------
 arch/arm/mach-imx/clock-imx35.c     |  536 ---------
 arch/arm/mach-imx/clock-imx6q.c     | 2111 -----------------------------------
 arch/arm/mach-imx/clock-mx51-mx53.c | 1675 ---------------------------
 8 files changed, 7958 deletions(-)
 delete mode 100644 arch/arm/mach-imx/clock-imx1.c
 delete mode 100644 arch/arm/mach-imx/clock-imx21.c
 delete mode 100644 arch/arm/mach-imx/clock-imx25.c
 delete mode 100644 arch/arm/mach-imx/clock-imx27.c
 delete mode 100644 arch/arm/mach-imx/clock-imx31.c
 delete mode 100644 arch/arm/mach-imx/clock-imx35.c
 delete mode 100644 arch/arm/mach-imx/clock-imx6q.c
 delete mode 100644 arch/arm/mach-imx/clock-mx51-mx53.c

diff --git a/arch/arm/mach-imx/clock-imx1.c b/arch/arm/mach-imx/clock-imx1.c
deleted file mode 100644
index 4aabeb2..0000000
diff --git a/arch/arm/mach-imx/clock-imx21.c b/arch/arm/mach-imx/clock-imx21.c
deleted file mode 100644
index ee15d8c..0000000
diff --git a/arch/arm/mach-imx/clock-imx25.c b/arch/arm/mach-imx/clock-imx25.c
deleted file mode 100644
index b0fec74c..0000000
diff --git a/arch/arm/mach-imx/clock-imx27.c b/arch/arm/mach-imx/clock-imx27.c
deleted file mode 100644
index 98e04f5..0000000
diff --git a/arch/arm/mach-imx/clock-imx31.c b/arch/arm/mach-imx/clock-imx31.c
deleted file mode 100644
index 3a943cd..0000000
diff --git a/arch/arm/mach-imx/clock-imx35.c b/arch/arm/mach-imx/clock-imx35.c
deleted file mode 100644
index e56c1a8..0000000
diff --git a/arch/arm/mach-imx/clock-imx6q.c b/arch/arm/mach-imx/clock-imx6q.c
deleted file mode 100644
index 111c328..0000000
diff --git a/arch/arm/mach-imx/clock-mx51-mx53.c b/arch/arm/mach-imx/clock-mx51-mx53.c
deleted file mode 100644
index 0847050..0000000
Russell King - ARM Linux April 10, 2012, 2:30 p.m. UTC | #2
On Tue, Apr 10, 2012 at 03:45:14PM +0200, Sascha Hauer wrote:
> With the generic clock framework we do not necessarily have
> a pointer to the struct clk we want to register a lookup
> for, so add a const char *clkname field to struct clk_lookup
> so that a lookup can be registered with a clock name.

All this silly names all over the place is getting to be utterly
rediculous. Why do we need to name clocks and look them up by name
when we have a perfectly good way (clkdev) to do this already?  Yes,
it takes a struct clk pointer but that's exactly because that's what
it has to return.

Why do we want to have another idiotic string to look up a clock which
is named using an idiotic scheme to find out its pointer to only then
register that with clkdev?

I think this clk stuff has gone totally insane wrt names recently.
Sascha Hauer April 10, 2012, 4:11 p.m. UTC | #3
On Tue, Apr 10, 2012 at 03:30:55PM +0100, Russell King - ARM Linux wrote:
> On Tue, Apr 10, 2012 at 03:45:14PM +0200, Sascha Hauer wrote:
> > With the generic clock framework we do not necessarily have
> > a pointer to the struct clk we want to register a lookup
> > for, so add a const char *clkname field to struct clk_lookup
> > so that a lookup can be registered with a clock name.
> 
> All this silly names all over the place is getting to be utterly
> rediculous. Why do we need to name clocks and look them up by name
> when we have a perfectly good way (clkdev) to do this already?  Yes,
> it takes a struct clk pointer but that's exactly because that's what
> it has to return.
> 
> Why do we want to have another idiotic string to look up a clock which
> is named using an idiotic scheme to find out its pointer to only then
> register that with clkdev?
> 
> I think this clk stuff has gone totally insane wrt names recently.

clk names and clk lookups are two different things. The former is a
unique identifier for a clock whereas the latter associates a clock
to a device. Multiple lookups can point to the same clock, so they can't
be used to identify a particular clock. I know I'm not telling anything
new to you here, so I think your question goes down to why we need a
unique identifier string for clocks.

For me the big advantage for clocks having unique names is that I do not
have to have a struct clk * to pass the (possible) parents of a clock to
the clock framework during registration. Also it makes us independent of
the registration order of the clock tree (no need to register the
parents before the children). The debugfs representation of the clock
tree also makes use of the names.

Sascha
Richard Zhao April 11, 2012, 1:11 a.m. UTC | #4
On Tue, Apr 10, 2012 at 06:11:42PM +0200, Sascha Hauer wrote:
> On Tue, Apr 10, 2012 at 03:30:55PM +0100, Russell King - ARM Linux wrote:
> > On Tue, Apr 10, 2012 at 03:45:14PM +0200, Sascha Hauer wrote:
> > > With the generic clock framework we do not necessarily have
> > > a pointer to the struct clk we want to register a lookup
> > > for, so add a const char *clkname field to struct clk_lookup
> > > so that a lookup can be registered with a clock name.
> > 
> > All this silly names all over the place is getting to be utterly
> > rediculous. Why do we need to name clocks and look them up by name
> > when we have a perfectly good way (clkdev) to do this already?  Yes,
> > it takes a struct clk pointer but that's exactly because that's what
> > it has to return.
> > 
> > Why do we want to have another idiotic string to look up a clock which
> > is named using an idiotic scheme to find out its pointer to only then
> > register that with clkdev?
> > 
> > I think this clk stuff has gone totally insane wrt names recently.
> 
> clk names and clk lookups are two different things. The former is a
> unique identifier for a clock whereas the latter associates a clock
> to a device. Multiple lookups can point to the same clock, so they can't
> be used to identify a particular clock. I know I'm not telling anything
> new to you here, so I think your question goes down to why we need a
> unique identifier string for clocks.
> 
> For me the big advantage for clocks having unique names is that I do not
> have to have a struct clk * to pass the (possible) parents of a clock to
> the clock framework during registration. Also it makes us independent of
> the registration order of the clock tree (no need to register the
> parents before the children). The debugfs representation of the clock
> tree also makes use of the names.
I don't like string loopup either. And after DT binding, we can use
phandler to refer clk.

BRs
Richard
Mark Brown April 11, 2012, 8:24 a.m. UTC | #5
On Wed, Apr 11, 2012 at 09:11:50AM +0800, Richard Zhao wrote:

> I don't like string loopup either. And after DT binding, we can use
> phandler to refer clk.

No, this is only useful on platforms that use DT.  This is a generic
Linux API so it needs to support architectures and platforms that don't
use DT as it's vanishingly unlikely that DT will ever be adopted by all
platforms.
Richard Zhao April 11, 2012, 8:45 a.m. UTC | #6
On Wed, Apr 11, 2012 at 09:24:02AM +0100, Mark Brown wrote:
> On Wed, Apr 11, 2012 at 09:11:50AM +0800, Richard Zhao wrote:
> 
> > I don't like string loopup either. And after DT binding, we can use
> > phandler to refer clk.
> 
> No, this is only useful on platforms that use DT.  This is a generic
> Linux API so it needs to support architectures and platforms that don't
> use DT as it's vanishingly unlikely that DT will ever be adopted by all
> platforms.
My point is using string lookup as less as possible. When one register
a clk, one already got the struct clk* pointer and could use it in
struct lookup.

I'm worried about the performance as I saw string lookup is used more
and more often. In fast boot case, for example, even 5ms is important.

Thanks
Richard
Mark Brown April 11, 2012, 9:15 a.m. UTC | #7
On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote:
> On Wed, Apr 11, 2012 at 09:24:02AM +0100, Mark Brown wrote:

> > No, this is only useful on platforms that use DT.  This is a generic
> > Linux API so it needs to support architectures and platforms that don't
> > use DT as it's vanishingly unlikely that DT will ever be adopted by all
> > platforms.

> My point is using string lookup as less as possible. When one register

That's not really relevant to what I'm saying here - I'm not commenting
on your goal, I'm commenting on the solution.  Obviously we want to be
able to use things with DT but if we rely on it then we still need to
solve the same problem for non-DT platforms.

> a clk, one already got the struct clk* pointer and could use it in
> struct lookup.

How will you handle cases where one clock supplies another?
Russell King - ARM Linux April 11, 2012, 9:20 a.m. UTC | #8
On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote:
> On Wed, Apr 11, 2012 at 09:24:02AM +0100, Mark Brown wrote:
> > On Wed, Apr 11, 2012 at 09:11:50AM +0800, Richard Zhao wrote:
> > 
> > > I don't like string loopup either. And after DT binding, we can use
> > > phandler to refer clk.
> > 
> > No, this is only useful on platforms that use DT.  This is a generic
> > Linux API so it needs to support architectures and platforms that don't
> > use DT as it's vanishingly unlikely that DT will ever be adopted by all
> > platforms.
> My point is using string lookup as less as possible. When one register
> a clk, one already got the struct clk* pointer and could use it in
> struct lookup.
> 
> I'm worried about the performance as I saw string lookup is used more
> and more often. In fast boot case, for example, even 5ms is important.

Yes, and you're not the only one who has that concern.  What this patch
does is turn a pair of string compares through a table into that plus
another set of string compares across all struct clk, which will make
the lookup yet more expensive.

I see no reason why you'd register the cl_lookup structures before you
know about which clks actually exist - and if they already exist, then
you can already find the struct clk pointer and use that to register
the proper return value for clk_get() via the clkdev APIs.

As I've said in the past, naming individual clock structures is the
stupidest thing that anyone ever did and causes nothing but problems.
We've seen that in the drivers with the abuse of clk_get(NULL, name)
where people wanted to pass the name in via platform data.  We're
starting to see the same thing here as well, except the problem people
will see will be boot performance instead caused by multiple levels of
string compares.
Russell King - ARM Linux April 11, 2012, 9:21 a.m. UTC | #9
On Wed, Apr 11, 2012 at 10:15:49AM +0100, Mark Brown wrote:
> On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote:
> > a clk, one already got the struct clk* pointer and could use it in
> > struct lookup.
> 
> How will you handle cases where one clock supplies another?

What has that got to do with clkdev (which is the topic in this thread)?
Mark Brown April 11, 2012, 9:32 a.m. UTC | #10
On Wed, Apr 11, 2012 at 10:21:47AM +0100, Russell King - ARM Linux wrote:
> On Wed, Apr 11, 2012 at 10:15:49AM +0100, Mark Brown wrote:
> > On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote:
> > > a clk, one already got the struct clk* pointer and could use it in
> > > struct lookup.

> > How will you handle cases where one clock supplies another?

> What has that got to do with clkdev (which is the topic in this thread)?

I'm expecting (well, hoping) that at some point we'll want to connect
clock trees between devices and it seems likely that clkdev will be
pressed into service for that since it's how we're binding clocks at the
minute.  It may be that we come up with some other scheme for making
those connections but as we appear to be defining a new scheme for
binding clocks together anyway it seems sensible that we should at least
be considering that case.
Russell King - ARM Linux April 11, 2012, 9:41 a.m. UTC | #11
On Wed, Apr 11, 2012 at 10:32:54AM +0100, Mark Brown wrote:
> On Wed, Apr 11, 2012 at 10:21:47AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Apr 11, 2012 at 10:15:49AM +0100, Mark Brown wrote:
> > > On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote:
> > > > a clk, one already got the struct clk* pointer and could use it in
> > > > struct lookup.
> 
> > > How will you handle cases where one clock supplies another?
> 
> > What has that got to do with clkdev (which is the topic in this thread)?
> 
> I'm expecting (well, hoping) that at some point we'll want to connect
> clock trees between devices and it seems likely that clkdev will be
> pressed into service for that since it's how we're binding clocks at the
> minute.  It may be that we come up with some other scheme for making
> those connections but as we appear to be defining a new scheme for
> binding clocks together anyway it seems sensible that we should at least
> be considering that case.

clkdev is just a mechanism to obtain a struct clk for a particular input
on a particular device.  Nothing more, nothing less.  So how does this
detail about how the struct clk ultimate gets used concern clkdev?
Sascha Hauer April 11, 2012, 9:42 a.m. UTC | #12
On Wed, Apr 11, 2012 at 10:20:34AM +0100, Russell King - ARM Linux wrote:
> On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote:
> > On Wed, Apr 11, 2012 at 09:24:02AM +0100, Mark Brown wrote:
> > > On Wed, Apr 11, 2012 at 09:11:50AM +0800, Richard Zhao wrote:
> > > 
> > > > I don't like string loopup either. And after DT binding, we can use
> > > > phandler to refer clk.
> > > 
> > > No, this is only useful on platforms that use DT.  This is a generic
> > > Linux API so it needs to support architectures and platforms that don't
> > > use DT as it's vanishingly unlikely that DT will ever be adopted by all
> > > platforms.
> > My point is using string lookup as less as possible. When one register
> > a clk, one already got the struct clk* pointer and could use it in
> > struct lookup.
> > 
> > I'm worried about the performance as I saw string lookup is used more
> > and more often. In fast boot case, for example, even 5ms is important.
> 
> Yes, and you're not the only one who has that concern.  What this patch
> does is turn a pair of string compares through a table into that plus
> another set of string compares across all struct clk, which will make
> the lookup yet more expensive.
> 
> I see no reason why you'd register the cl_lookup structures before you
> know about which clks actually exist - and if they already exist, then
> you can already find the struct clk pointer and use that to register
> the proper return value for clk_get() via the clkdev APIs.

Indeed I have struct clk pointers, so I can instead use a to-be-written
combination of clkdev_alloc/clkdev_add. Would that be ok?

Sascha
Russell King - ARM Linux April 11, 2012, 9:47 a.m. UTC | #13
On Wed, Apr 11, 2012 at 11:42:31AM +0200, Sascha Hauer wrote:
> On Wed, Apr 11, 2012 at 10:20:34AM +0100, Russell King - ARM Linux wrote:
> > Yes, and you're not the only one who has that concern.  What this patch
> > does is turn a pair of string compares through a table into that plus
> > another set of string compares across all struct clk, which will make
> > the lookup yet more expensive.
> > 
> > I see no reason why you'd register the cl_lookup structures before you
> > know about which clks actually exist - and if they already exist, then
> > you can already find the struct clk pointer and use that to register
> > the proper return value for clk_get() via the clkdev APIs.
> 
> Indeed I have struct clk pointers, so I can instead use a to-be-written
> combination of clkdev_alloc/clkdev_add. Would that be ok?

As we already have clkdev_alloc and clkdev_add, then I don't see that as
a problem - except we may have to change __clkdev_alloc() so that it can
be used before kmalloc() is up and running.
Mark Brown April 11, 2012, 10:31 a.m. UTC | #14
On Wed, Apr 11, 2012 at 10:41:36AM +0100, Russell King - ARM Linux wrote:

> clkdev is just a mechanism to obtain a struct clk for a particular input
> on a particular device.  Nothing more, nothing less.  So how does this
> detail about how the struct clk ultimate gets used concern clkdev?

It shouldn't.  

The main point I'm trying to make is that if we're adding an alternative
mechanism to the direct pointers for specifying the bindings to clkdev
(which is broadly the point of the original patch, AFAICT based on a
desire to split the mapping out from the definitions) then having it rely
on device tree in the first instance means we'll just need to solve the
same problem again for non-DT systems.  Richard was saying that the
problem Sascha was trying to solve is irrelevant because we can rely on
device tree but device tree is not widely available enough for that.

The bit about clock to clock mappings was mostly an aside since it seems
like it's roughly the same as what Sascha was trying to do and if we're
going to add something new it'd be better if it handled that case also;
that said re-reading Richard's message I'm not sure if this is actually
a suggestion of a new mechanism.
Russell King - ARM Linux April 11, 2012, 11:43 a.m. UTC | #15
On Wed, Apr 11, 2012 at 11:31:07AM +0100, Mark Brown wrote:
> On Wed, Apr 11, 2012 at 10:41:36AM +0100, Russell King - ARM Linux wrote:
> > clkdev is just a mechanism to obtain a struct clk for a particular input
> > on a particular device.  Nothing more, nothing less.  So how does this
> > detail about how the struct clk ultimate gets used concern clkdev?
> 
> It shouldn't.  
> 
> The main point I'm trying to make is that if we're adding an alternative
> mechanism to the direct pointers for specifying the bindings to clkdev
> (which is broadly the point of the original patch, AFAICT based on a
> desire to split the mapping out from the definitions) then having it rely
> on device tree in the first instance means we'll just need to solve the
> same problem again for non-DT systems.  Richard was saying that the
> problem Sascha was trying to solve is irrelevant because we can rely on
> device tree but device tree is not widely available enough for that.

Err what?  Look, it's very very simple.

clkdev is all about translating a device + connection ID to a struct clk
pointer.  That's all, nothing more, nothing less.

What I see going on is that people want to translate a device + connection
ID to some other name, and use this other name to look up a struct clk.
Utterly idiotic and pointless.  Whether it be OF or not OF.

It's a crazy absurd idea.  At some point, you have to do some kind of
lookup and return a struct clk.  Why the hell have this insane double
mapping?

Look, solving the DT problem with clkdev is really simple.  You have the
DT node for the struct device, so you can look up DT properties associated
with that device.  Properties have a name, and you can use the connection
ID to translate into a DT property name to be looked up - eg, clk_%s.
This can return a phandle or whatever else which could then be translated
into a struct clk if you're representing each clk as a node in DT.

Nice and simple, no need for any additional crappy translation what so
ever - and, oh my god, it reduces the amount of list based searching
required to do the translation.

And it also allows non-DT to work just fine provided you register your
clk lookups _after_ you know the struct clk pointers which is exactly what
happens today.

Now, if getting the struct clk pointers is insanely difficult because of
the design of the common clk stuff, then that's where the problem lies,
and that problem needs to be fixed, and clkdev does not need to be bodged
around to fix a problem which is not relevant to it.
Mark Brown April 12, 2012, 4:33 p.m. UTC | #16
On Wed, Apr 11, 2012 at 12:43:11PM +0100, Russell King - ARM Linux wrote:

> What I see going on is that people want to translate a device + connection
> ID to some other name, and use this other name to look up a struct clk.
> Utterly idiotic and pointless.  Whether it be OF or not OF.

> It's a crazy absurd idea.  At some point, you have to do some kind of
> lookup and return a struct clk.  Why the hell have this insane double
> mapping?

I agree, that is silly.  I had understood from Sascha's comments that
the use case was building up the SoC clock tree with some indirect
references.  I can understand someone wanting to do that for a use case
like grafting chunks of tree built up of static data together, it might
be a bit more nicely data driven.

> Look, solving the DT problem with clkdev is really simple.  You have the
> DT node for the struct device, so you can look up DT properties associated

Yes, DT is completely straightforward here.

> And it also allows non-DT to work just fine provided you register your
> clk lookups _after_ you know the struct clk pointers which is exactly what
> happens today.

> Now, if getting the struct clk pointers is insanely difficult because of
> the design of the common clk stuff, then that's where the problem lies,
> and that problem needs to be fixed, and clkdev does not need to be bodged
> around to fix a problem which is not relevant to it.

I don't think it's anything particularly to do with the common clk stuff
really (at least I share your opinion that it shouldn't be) but it does
seem like it might be a real pain once we start deploying clock trees
that go off SoC.
Viresh KUMAR April 13, 2012, 3:33 a.m. UTC | #17
On 4/11/2012 3:17 PM, Russell King - ARM Linux wrote:
>> > Indeed I have struct clk pointers, so I can instead use a to-be-written
>> > combination of clkdev_alloc/clkdev_add. Would that be ok?
> As we already have clkdev_alloc and clkdev_add, then I don't see that as
> a problem - except we may have to change __clkdev_alloc() so that it can
> be used before kmalloc() is up and running.

Instead of platforms calling these routines, can we have these calls from
clk_register_*() routines directly? So, for every clock registered with
common clk framework this gets done automatically. We just need to pass
dev & con id strings to these routines.
Shawn Guo April 13, 2012, 4:17 a.m. UTC | #18
On 13 April 2012 11:33, Viresh Kumar <viresh.kumar@st.com> wrote:
> On 4/11/2012 3:17 PM, Russell King - ARM Linux wrote:
>>> > Indeed I have struct clk pointers, so I can instead use a to-be-written
>>> > combination of clkdev_alloc/clkdev_add. Would that be ok?
>> As we already have clkdev_alloc and clkdev_add, then I don't see that as
>> a problem - except we may have to change __clkdev_alloc() so that it can
>> be used before kmalloc() is up and running.
>
> Instead of platforms calling these routines, can we have these calls from
> clk_register_*() routines directly? So, for every clock registered with
> common clk framework this gets done automatically. We just need to pass
> dev & con id strings to these routines.
>
No.

Only small portion of the entire clock tree need to be in the lookup,
usually the leaf clocks that device drivers need to access/manage.

Regards,
Shawn
Viresh KUMAR April 13, 2012, 5:06 a.m. UTC | #19
On 4/13/2012 9:47 AM, Shawn Guo wrote:
>> >
>> > Instead of platforms calling these routines, can we have these calls from
>> > clk_register_*() routines directly? So, for every clock registered with
>> > common clk framework this gets done automatically. We just need to pass
>> > dev & con id strings to these routines.
>> >
> No.
> 
> Only small portion of the entire clock tree need to be in the lookup,
> usually the leaf clocks that device drivers need to access/manage.

I am not asking to enforce this for all clocks and create too many clk lookups.
But do it for all clk_register() calls, that have valid dev_id or con_id.

Apart from device clocks, there are other clocks, parent selection, etc
that need it, so that we can do clk_get_sys().
Shawn Guo April 13, 2012, 5:22 a.m. UTC | #20
On 13 April 2012 13:06, Viresh Kumar <viresh.kumar@st.com> wrote:
> On 4/13/2012 9:47 AM, Shawn Guo wrote:
>>> >
>>> > Instead of platforms calling these routines, can we have these calls from
>>> > clk_register_*() routines directly? So, for every clock registered with
>>> > common clk framework this gets done automatically. We just need to pass
>>> > dev & con id strings to these routines.
>>> >
>> No.
>>
>> Only small portion of the entire clock tree need to be in the lookup,
>> usually the leaf clocks that device drivers need to access/manage.
>
> I am not asking to enforce this for all clocks and create too many clk lookups.
> But do it for all clk_register() calls, that have valid dev_id or con_id.
>
It makes sense then.  Only concern I have is the long argument list of
clk_register() becomes even longer.

Regards,
Shawn

> Apart from device clocks, there are other clocks, parent selection, etc
> that need it, so that we can do clk_get_sys().
Mark Brown April 13, 2012, 8:59 a.m. UTC | #21
On Fri, Apr 13, 2012 at 01:22:24PM +0800, Shawn Guo wrote:

> It makes sense then.  Only concern I have is the long argument list of
> clk_register() becomes even longer.

Or we need to dump the arguments into a struct.
Viresh KUMAR April 13, 2012, 9:10 a.m. UTC | #22
On 4/13/2012 2:29 PM, Mark Brown wrote:
> Or we need to dump the arguments into a struct.

Then we would end up creating too many structures in our mach/clock.c
files. That will look bad. May perform better. :)
Richard Zhao April 13, 2012, 9:17 a.m. UTC | #23
On Fri, Apr 13, 2012 at 02:40:12PM +0530, Viresh Kumar wrote:
> On 4/13/2012 2:29 PM, Mark Brown wrote:
> > Or we need to dump the arguments into a struct.
> 
> Then we would end up creating too many structures in our mach/clock.c
> files. That will look bad. May perform better. :)
or pass struct clk_lookup* ? For non-leaf clks, it's NULL.

> 
> -- 
> viresh
>
Russell King - ARM Linux April 13, 2012, 9:26 a.m. UTC | #24
On Fri, Apr 13, 2012 at 05:17:13PM +0800, Richard Zhao wrote:
> On Fri, Apr 13, 2012 at 02:40:12PM +0530, Viresh Kumar wrote:
> > On 4/13/2012 2:29 PM, Mark Brown wrote:
> > > Or we need to dump the arguments into a struct.
> > 
> > Then we would end up creating too many structures in our mach/clock.c
> > files. That will look bad. May perform better. :)
> or pass struct clk_lookup* ? For non-leaf clks, it's NULL.

What's wrong with the struct clk returned from clk_register() - why
not do this as a two-step process?  Especially as you may want to
associate a single clock with more than one clk_lookup.
Viresh KUMAR April 13, 2012, 9:27 a.m. UTC | #25
On 4/13/2012 2:56 PM, Russell King - ARM Linux wrote:
> What's wrong with the struct clk returned from clk_register() - why
> not do this as a two-step process?

Code duplication on all platforms for creating these lookups.

> Especially as you may want to
> associate a single clock with more than one clk_lookup.

Can still be done, as we are still getting clk pointer back.
Russell King - ARM Linux April 13, 2012, 9:36 a.m. UTC | #26
On Fri, Apr 13, 2012 at 02:57:36PM +0530, Viresh Kumar wrote:
> On 4/13/2012 2:56 PM, Russell King - ARM Linux wrote:
> > What's wrong with the struct clk returned from clk_register() - why
> > not do this as a two-step process?
> 
> Code duplication on all platforms for creating these lookups.
> 
> > Especially as you may want to
> > associate a single clock with more than one clk_lookup.
> 
> Can still be done, as we are still getting clk pointer back.

You're not convincing me that you're approach here is correct, and
I really doubt that this will have any effect at saving lines of code -
your argument list is soo long that you'll have to wrap it onto several
lines.

So I don't buy the 'code duplication' argument here - I think that's a
red herring, and I think you're better off making interfaces which are
simpler to use.
Viresh KUMAR April 13, 2012, 10:02 a.m. UTC | #27
On 4/13/2012 3:06 PM, Russell King - ARM Linux wrote:
> You're not convincing me that you're approach here is correct, and

:(

> I really doubt that this will have any effect at saving lines of code -
> your argument list is soo long that you'll have to wrap it onto several
> lines.

There are two ways here:
1. Leave clk_register() untouched and create lookups in machine code.
2. change clk_register():
	A. pass all arguments as i mentioned in my patch
	B. pass structure instead of all so many args.

Option 2 will have much lesser code than option 1, with both option A & B.
In option 2.A.: we can create separate registration routines to save extra
arguments passed, like:

 struct clk *clk_register_gate(struct device *dev, const char *name,
 		const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 bit_idx,
		u8 clk_gate_flags, spinlock_t *lock, struct clk_lookup **cl,
		const char *dev_id, const char *con_id);

static inline struct clk *clk_register_gate_nolookup(struct device *dev,
		const char *name, const char *parent_name, unsigned long flags,
		void __iomem *reg, u8 bit_idx, u8 clk_gate_flags,
		spinlock_t *lock);

Nodes, that don't need lookup stuff, can go ahead as earlier, just need
to add _nolookup in existing routine name.
Mark Brown April 13, 2012, 10:08 a.m. UTC | #28
On Fri, Apr 13, 2012 at 03:32:43PM +0530, Viresh Kumar wrote:

> Nodes, that don't need lookup stuff, can go ahead as earlier, just need
> to add _nolookup in existing routine name.

Or just have the core function handle NULL as a lookup array gracefully.
Russell King - ARM Linux April 13, 2012, 10:20 a.m. UTC | #29
On Fri, Apr 13, 2012 at 03:32:43PM +0530, Viresh Kumar wrote:
> On 4/13/2012 3:06 PM, Russell King - ARM Linux wrote:
> > You're not convincing me that you're approach here is correct, and
> 
> :(
> 
> > I really doubt that this will have any effect at saving lines of code -
> > your argument list is soo long that you'll have to wrap it onto several
> > lines.
> 
> There are two ways here:
> 1. Leave clk_register() untouched and create lookups in machine code.
> 2. change clk_register():
> 	A. pass all arguments as i mentioned in my patch
> 	B. pass structure instead of all so many args.
> 
> Option 2 will have much lesser code than option 1, with both option A & B.
> In option 2.A.: we can create separate registration routines to save extra
> arguments passed, like:
> 
>  struct clk *clk_register_gate(struct device *dev, const char *name,
>  		const char *parent_name, unsigned long flags,
>  		void __iomem *reg, u8 bit_idx,
> 		u8 clk_gate_flags, spinlock_t *lock, struct clk_lookup **cl,
> 		const char *dev_id, const char *con_id);

So you're going to have something around 4-5 lines of arguments per
function call to use this function.  That's not elegant, that's not
easy to use, that's not nice.  And I doubt that it really solves any
code space concern because the compiler will have to store pointers
and values somewhere, and shuffle those into registers and onto the
stack for every function call using additional code to do that.

I really don't get why you think this is an improvement.
Viresh KUMAR April 13, 2012, 10:43 a.m. UTC | #30
On 4/13/2012 3:50 PM, Russell King - ARM Linux wrote:
> So you're going to have something around 4-5 lines of arguments per
> function call to use this function.  That's not elegant, that's not
> easy to use, that's not nice.  And I doubt that it really solves any
> code space concern because the compiler will have to store pointers
> and values somewhere, and shuffle those into registers and onto the
> stack for every function call using additional code to do that.

But it is still the same with the current implementation. Right now
also we have too many arguments. Even if we do clkdev_alloc() in
platform files.

> I really don't get why you think this is an improvement.

Only improvement is simplicity at the end of machine clock.c files, that
don't need to do clkdev_alloc() for every clock they clk_lookups.

I didn't liked this long of argument list, but couldn't think of
something better, that makes machine clock code easy. :(
Turquette, Mike April 13, 2012, 11:19 p.m. UTC | #31
On Fri, Apr 13, 2012 at 3:20 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Apr 13, 2012 at 03:32:43PM +0530, Viresh Kumar wrote:
>> On 4/13/2012 3:06 PM, Russell King - ARM Linux wrote:
>> > You're not convincing me that you're approach here is correct, and
>>
>> :(
>>
>> > I really doubt that this will have any effect at saving lines of code -
>> > your argument list is soo long that you'll have to wrap it onto several
>> > lines.
>>
>> There are two ways here:
>> 1. Leave clk_register() untouched and create lookups in machine code.
>> 2. change clk_register():
>>       A. pass all arguments as i mentioned in my patch
>>       B. pass structure instead of all so many args.
>>
>> Option 2 will have much lesser code than option 1, with both option A & B.
>> In option 2.A.: we can create separate registration routines to save extra
>> arguments passed, like:
>>
>>  struct clk *clk_register_gate(struct device *dev, const char *name,
>>               const char *parent_name, unsigned long flags,
>>               void __iomem *reg, u8 bit_idx,
>>               u8 clk_gate_flags, spinlock_t *lock, struct clk_lookup **cl,
>>               const char *dev_id, const char *con_id);
>
> So you're going to have something around 4-5 lines of arguments per
> function call to use this function.  That's not elegant, that's not
> easy to use, that's not nice.  And I doubt that it really solves any
> code space concern because the compiler will have to store pointers
> and values somewhere, and shuffle those into registers and onto the
> stack for every function call using additional code to do that.
>
> I really don't get why you think this is an improvement.

My hope is that forthcoming struct clk_hw/static initializer patch
will reduce the number of arguments to clk_register considerably.
These interfaces are not frozen and everyone concerned with using the
common framework is still figuring out exactly how this stuff should
look.

http://article.gmane.org/gmane.linux.ports.arm.msm/2587

Regards,
Mike
Domenico Andreoli April 19, 2012, 6:16 a.m. UTC | #32
On Thu, Apr 19, 2012 at 09:18:01AM +0530, Viresh Kumar wrote:
> On 4/10/2012 7:15 PM, Sascha Hauer wrote:
> > Having fixed factors/dividers in hardware is a common pattern, so
> > add a basic clock type doing this. It basically describes a fixed
> > factor clock using a nominator and a denominator.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/clk/Makefile           |    2 +-
> >  drivers/clk/clk-fixed-factor.c |   97 ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/clk-provider.h   |    4 ++
> >  3 files changed, 102 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/clk/clk-fixed-factor.c
> 
> @Mike: It would be better if you can take this patch atleast ASAP in your next,
> so that Arnd can pull in SPEAr clk patches.
> 
> Hi Sascha/Mike,
> 
> Please see if following can be squashed with this patch. This clock is
> used for SPEAr too :)
> 
> ---
>  drivers/clk/clk-fixed-factor.c |   12 ++----------
>  1 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
> index 7c5e1fc..aaf5a88 100644
> --- a/drivers/clk/clk-fixed-factor.c
> +++ b/drivers/clk/clk-fixed-factor.c
> @@ -16,7 +16,6 @@ struct clk_fixed_factor {
>  	struct clk_hw	hw;
>  	unsigned int	mult;
>  	unsigned int	div;
> -	char		*parent[1];

add const here

>  };
>  
>  #define to_clk_fixed_factor(_hw) container_of(_hw, struct clk_fixed_factor, hw)
> @@ -75,22 +74,15 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
>  	fix->mult = mult;
>  	fix->div = div;
>  
> -	if (parent_name) {

		char *tmp = kstrdup(parent_name, GFP_KERNEL);
		if (!tmp)
			goto out;
		fix->parent[0] = tmp;
	
> -	}
> -
>  	clk = clk_register(dev, name,
>  			&clk_fixed_factor_ops, &fix->hw,
> -			fix->parent,
> +			&parent_name,

&parent_name is a pointer to a stack variable, right? clk_register
saves it in the clk struct for later use, when the stack is already gone.

>  			(parent_name ? 1 : 0),
>  			flags);
> +
>  	if (clk)
>  		return clk;
>  
> -out:
> -	kfree(fix->parent[0]);
>  	kfree(fix);
>  
>  	return NULL;

BRs,
Domenico
Viresh KUMAR April 19, 2012, 6:19 a.m. UTC | #33
On 4/19/2012 11:46 AM, Domenico Andreoli wrote:
> On Thu, Apr 19, 2012 at 09:18:01AM +0530, Viresh Kumar wrote:
>> On 4/10/2012 7:15 PM, Sascha Hauer wrote:
>> diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
>> @@ -16,7 +16,6 @@ struct clk_fixed_factor {
>>  	struct clk_hw	hw;
>>  	unsigned int	mult;
>>  	unsigned int	div;
>> -	char		*parent[1];
> 
> add const here
> 

I removed it. :)

>>  	clk = clk_register(dev, name,
>>  			&clk_fixed_factor_ops, &fix->hw,
>> -			fix->parent,
>> +			&parent_name,
> 
> &parent_name is a pointer to a stack variable, right? clk_register
> saves it in the clk struct for later use, when the stack is already gone.

Yes. It is on stack, but clk_register doesn't save it any more. Following is
part of clk_register that saves

	/* copy each string name in case parent_names is __initdata */
	for (i = 0; i < num_parents; i++) {
		clk->parent_names[i] = kstrdup(parent_names[i], GFP_KERNEL);
		if (!clk->parent_names[i]) {
			pr_err("%s: could not copy parent_names\n", __func__);
			goto fail_parent_names_copy;
		}
	}

This happened with following patch sent by Mike. This isn't pushed till now.

Author: Mike Turquette <mturquette@linaro.org>
Date:   Thu Apr 12 09:02:50 2012 +0800

    clk: core: copy parent_names & return error codes
    
    This patch cleans up clk_register and solves a few bugs by teaching
    clk_register and __clk_init to return error codes (instead of just NULL)
    to better align with the existing clk.h api.
    
    Along with that change this patch also introduces a new behavior whereby
    clk_register copies the parent_names array, thus allowing platforms to
    declare their parent_names arrays as __initdata.

--
Viresh
Sascha Hauer April 19, 2012, 6:45 a.m. UTC | #34
On Thu, Apr 19, 2012 at 09:18:01AM +0530, Viresh Kumar wrote:
> On 4/10/2012 7:15 PM, Sascha Hauer wrote:
> > Having fixed factors/dividers in hardware is a common pattern, so
> > add a basic clock type doing this. It basically describes a fixed
> > factor clock using a nominator and a denominator.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/clk/Makefile           |    2 +-
> >  drivers/clk/clk-fixed-factor.c |   97 ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/clk-provider.h   |    4 ++
> >  3 files changed, 102 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/clk/clk-fixed-factor.c
> 
> @Mike: It would be better if you can take this patch atleast ASAP in your next,
> so that Arnd can pull in SPEAr clk patches.
> 
> Hi Sascha/Mike,
> 
> Please see if following can be squashed with this patch. This clock is
> used for SPEAr too :)

Generally yes, but I want to delay this until we have an official branch
for clk-common-next (In which probably clk_register copies the parent
names)

Sascha

> 
> ---
>  drivers/clk/clk-fixed-factor.c |   12 ++----------
>  1 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
> index 7c5e1fc..aaf5a88 100644
> --- a/drivers/clk/clk-fixed-factor.c
> +++ b/drivers/clk/clk-fixed-factor.c
> @@ -16,7 +16,6 @@ struct clk_fixed_factor {
>  	struct clk_hw	hw;
>  	unsigned int	mult;
>  	unsigned int	div;
> -	char		*parent[1];
>  };
>  
>  #define to_clk_fixed_factor(_hw) container_of(_hw, struct clk_fixed_factor, hw)
> @@ -75,22 +74,15 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
>  	fix->mult = mult;
>  	fix->div = div;
>  
> -	if (parent_name) {
> -		fix->parent[0] = kstrdup(parent_name, GFP_KERNEL);
> -		if (!fix->parent[0])
> -			goto out;
> -	}
> -
>  	clk = clk_register(dev, name,
>  			&clk_fixed_factor_ops, &fix->hw,
> -			fix->parent,
> +			&parent_name,
>  			(parent_name ? 1 : 0),
>  			flags);
> +
>  	if (clk)
>  		return clk;
>  
> -out:
> -	kfree(fix->parent[0]);
>  	kfree(fix);
>  
>  	return NULL;
> -- 
> 1.7.9
>
Richard Zhao April 20, 2012, 2:06 a.m. UTC | #35
On Tue, Apr 10, 2012 at 03:45:22PM +0200, Sascha Hauer wrote:
> the current i.MX clock support groups together unrelated clocks
> to a single clock which is then used by the driver. This can't
> be accomplished with the generic clock framework so we instead
> request the individual clocks in the driver. For i.MX there are
> generally three different clocks:
> 
> ipg: bus clock (needed to access registers)
> ahb: dma relevant clock, sometimes referred to as hclk in the datasheet
> per: bit clock, pixel clock
> 
> This patch changes the driver to request the individual clocks.
> Currently all clk_get will get the same clock until the SoCs
> are converted to the generic clock framework
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c |   42 ++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 6193a0d..938095a 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -69,6 +69,9 @@ struct pltfm_imx_data {
>  	u32 scratchpad;
>  	enum imx_esdhc_type devtype;
>  	struct esdhc_platform_data boarddata;
> +	struct clk *clk_ipg;
> +	struct clk *clk_ahb;
> +	struct clk *clk_per;
>  };
>  
>  static struct platform_device_id imx_esdhc_devtype[] = {
> @@ -437,7 +440,6 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  	struct sdhci_pltfm_host *pltfm_host;
>  	struct sdhci_host *host;
>  	struct esdhc_platform_data *boarddata;
> -	struct clk *clk;
>  	int err;
>  	struct pltfm_imx_data *imx_data;
>  
> @@ -458,14 +460,29 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  	imx_data->devtype = pdev->id_entry->driver_data;
>  	pltfm_host->priv = imx_data;
>  
> -	clk = clk_get(mmc_dev(host->mmc), NULL);
> -	if (IS_ERR(clk)) {
> -		dev_err(mmc_dev(host->mmc), "clk err\n");
> -		err = PTR_ERR(clk);
> +	imx_data->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> +	if (IS_ERR(imx_data->clk_ipg)) {
> +		err = PTR_ERR(imx_data->clk_ipg);
>  		goto err_clk_get;
>  	}
> -	clk_prepare_enable(clk);
> -	pltfm_host->clk = clk;
> +
> +	imx_data->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
> +	if (IS_ERR(imx_data->clk_ahb)) {
> +		err = PTR_ERR(imx_data->clk_ahb);
> +		goto err_clk_get;
> +	}
> +
> +	imx_data->clk_per = devm_clk_get(&pdev->dev, "per");
> +	if (IS_ERR(imx_data->clk_per)) {
> +		err = PTR_ERR(imx_data->clk_per);
> +		goto err_clk_get;
> +	}
> +
> +	pltfm_host->clk = imx_data->clk_per;
> +
> +	clk_prepare_enable(imx_data->clk_per);
> +	clk_prepare_enable(imx_data->clk_ipg);
> +	clk_prepare_enable(imx_data->clk_ahb);
>  
>  	if (!is_imx25_esdhc(imx_data))
>  		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
> @@ -559,8 +576,9 @@ no_card_detect_irq:
>  		gpio_free(boarddata->wp_gpio);
>  no_card_detect_pin:
>  no_board_data:
> -	clk_disable_unprepare(pltfm_host->clk);
> -	clk_put(pltfm_host->clk);
> +	clk_disable_unprepare(imx_data->clk_per);
> +	clk_disable_unprepare(imx_data->clk_ipg);
> +	clk_disable_unprepare(imx_data->clk_ahb);
You forgot to clk_put them.
>  err_clk_get:
>  	kfree(imx_data);
>  err_imx_data:
> @@ -586,8 +604,10 @@ static int __devexit sdhci_esdhc_imx_remove(struct platform_device *pdev)
>  		gpio_free(boarddata->cd_gpio);
>  	}
>  
> -	clk_disable_unprepare(pltfm_host->clk);
> -	clk_put(pltfm_host->clk);
> +	clk_disable_unprepare(imx_data->clk_per);
> +	clk_disable_unprepare(imx_data->clk_ipg);
> +	clk_disable_unprepare(imx_data->clk_ahb);
ditto

Thanks
Richard
> +
>  	kfree(imx_data);
>  
>  	sdhci_pltfm_free(pdev);
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Richard Zhao April 20, 2012, 2:42 a.m. UTC | #36
> > -	clk_disable_unprepare(pltfm_host->clk);
> > -	clk_put(pltfm_host->clk);
> > +	clk_disable_unprepare(imx_data->clk_per);
> > +	clk_disable_unprepare(imx_data->clk_ipg);
> > +	clk_disable_unprepare(imx_data->clk_ahb);
> You forgot to clk_put them.
Sorry, I just realized you're using the new devm_clk_get.

Richard
Richard Zhao April 24, 2012, 2:17 a.m. UTC | #37
On Wed, Apr 11, 2012 at 10:20:34AM +0100, Russell King - ARM Linux wrote:
> On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote:
> > On Wed, Apr 11, 2012 at 09:24:02AM +0100, Mark Brown wrote:
> > > On Wed, Apr 11, 2012 at 09:11:50AM +0800, Richard Zhao wrote:
> > > 
> > > > I don't like string loopup either. And after DT binding, we can use
> > > > phandler to refer clk.
> > > 
> > > No, this is only useful on platforms that use DT.  This is a generic
> > > Linux API so it needs to support architectures and platforms that don't
> > > use DT as it's vanishingly unlikely that DT will ever be adopted by all
> > > platforms.
> > My point is using string lookup as less as possible. When one register
> > a clk, one already got the struct clk* pointer and could use it in
> > struct lookup.
> > 
> > I'm worried about the performance as I saw string lookup is used more
> > and more often. In fast boot case, for example, even 5ms is important.
> 
> Yes, and you're not the only one who has that concern.  What this patch
> does is turn a pair of string compares through a table into that plus
> another set of string compares across all struct clk, which will make
> the lookup yet more expensive.
> 
> I see no reason why you'd register the cl_lookup structures before you
> know about which clks actually exist - and if they already exist, then
> you can already find the struct clk pointer and use that to register
> the proper return value for clk_get() via the clkdev APIs.
> 
> As I've said in the past, naming individual clock structures is the
> stupidest thing that anyone ever did and causes nothing but problems.
> We've seen that in the drivers with the abuse of clk_get(NULL, name)
> where people wanted to pass the name in via platform data.  We're
> starting to see the same thing here as well, except the problem people
> will see will be boot performance instead caused by multiple levels of
> string compares.
I just realized an important benefit of string lookup is we can define
machine specific lookups at its machine file.
Without it, we have to expose machine specific clk twice. For example,
cko1 is SoC internal clock name and is connected to sgtl5000 audio codec.
{"sgtl5000-dev-name", "function name"} lookup is board specific, and
{NULL, "cko1"} is SoC specific. We have to expose both of them.

Thanks
Richard
>