All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] sh: switch various MSTP PM clocks to device-ID look-up
@ 2012-04-26 21:48 Guennadi Liakhovetski
  2012-04-27  0:31 ` Paul Mundt
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Guennadi Liakhovetski @ 2012-04-26 21:48 UTC (permalink / raw)
  To: linux-sh

Most SH drivers, accessing clocks, used for their devices, directly, have
been converted to not use connection IDs for clock look-up. The runtime PM
subsystem does the same. This means, that clock look-up entries for such
devices with non-NULL connection IDs are useless. This patch converts such
clock look-up entries to correct device IDs.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

This supersedes my earlier patch "[PATCH 1/6] sh: switch MSIOF and MMCIF 
PM clocks to device-ID look-up"
http://www.spinics.net/lists/linux-sh/msg11015.html
After a more detailed look, I think, it is best to just convert these 
entries to CLKDEV_DEV_ID(), instead of adding them to existing 
CLKDEV_CON_ID() ones. It does look like the present entries are 
non-functional and keeping them around makes no sense. Most of these 
lookup entries are also currently unused - affected platforms don't 
register respective devices. The only used ones are
1. MMCIF on sh7757
2. DMA on sh7785 and sh7786
(1) will be broken without this patch, once "[PATCH 5/6] mmc: mmcif: don't 
use connection IDs to obtain a clock reference" is committed. So, this 
patch is required to keep MMCIF working on sh7757, and can actually be 
committed before the above patch, nothing would break. (2) won't affect 
the DMAC driver - it doesn't acquire clocks directly. The legacy SH DMA 
driver under arch/sh/drivers/dma/ doesn't use the clock API either.

What will be affected - is the runtime PM. It will begin to work:-) So, if 
any of these platforms have problems in that area, they can become 
apparent.

Another thing to note - I didn't convert TMU entries on sh7343 and sh7366. 
On both these platforms currently there's one tmu clock look-up entry with 
just a "tmu_fck" connection ID, but platforms themselves register multiple 
tmu devices. Without datasheets I have no idea, whether some clock entries 
are missing from the lookup table, or that entry controlls all TMU clocks, 
or only one (#0?) TMU clock can be switched on and off.

 arch/sh/kernel/cpu/sh4a/clock-sh7343.c |    8 ++++----
 arch/sh/kernel/cpu/sh4a/clock-sh7366.c |   12 ++++++------
 arch/sh/kernel/cpu/sh4a/clock-sh7757.c |    2 +-
 arch/sh/kernel/cpu/sh4a/clock-sh7785.c |    6 +++---
 arch/sh/kernel/cpu/sh4a/clock-sh7786.c |    8 ++++----
 arch/sh/kernel/cpu/sh4a/clock-shx3.c   |    4 ++--
 6 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/arch/sh/kernel/cpu/sh4a/clock-sh7343.c b/arch/sh/kernel/cpu/sh4a/clock-sh7343.c
index ea01a72..0e8904b 100644
--- a/arch/sh/kernel/cpu/sh4a/clock-sh7343.c
+++ b/arch/sh/kernel/cpu/sh4a/clock-sh7343.c
@@ -244,10 +244,10 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_DEV_ID("i2c-sh_mobile.1", &mstp_clks[MSTP108]),
 	CLKDEV_CON_ID("tpu0", &mstp_clks[MSTP225]),
 	CLKDEV_CON_ID("irda0", &mstp_clks[MSTP224]),
-	CLKDEV_CON_ID("sdhi0", &mstp_clks[MSTP218]),
-	CLKDEV_CON_ID("mmcif0", &mstp_clks[MSTP217]),
+	CLKDEV_DEV_ID("sh_mobile_sdhi.0", &mstp_clks[MSTP218]),
+	CLKDEV_DEV_ID("sh_mmcif.0", &mstp_clks[MSTP217]),
 	CLKDEV_CON_ID("sim0", &mstp_clks[MSTP216]),
-	CLKDEV_CON_ID("keysc0", &mstp_clks[MSTP214]),
+	CLKDEV_DEV_ID("sh_keysc.0", &mstp_clks[MSTP214]),
 	CLKDEV_CON_ID("tsif0", &mstp_clks[MSTP213]),
 	CLKDEV_CON_ID("s3d40", &mstp_clks[MSTP212]),
 	CLKDEV_CON_ID("usbf0", &mstp_clks[MSTP211]),
@@ -258,7 +258,7 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_CON_ID("ceu0", &mstp_clks[MSTP203]),
 	CLKDEV_CON_ID("veu0", &mstp_clks[MSTP202]),
 	CLKDEV_CON_ID("vpu0", &mstp_clks[MSTP201]),
-	CLKDEV_CON_ID("lcdc0", &mstp_clks[MSTP200]),
+	CLKDEV_DEV_ID("sh_mobile_lcdc_fb.0", &mstp_clks[MSTP200]),
 };
 
 int __init arch_clk_init(void)
diff --git a/arch/sh/kernel/cpu/sh4a/clock-sh7366.c b/arch/sh/kernel/cpu/sh4a/clock-sh7366.c
index 7ac07b4..fad5588 100644
--- a/arch/sh/kernel/cpu/sh4a/clock-sh7366.c
+++ b/arch/sh/kernel/cpu/sh4a/clock-sh7366.c
@@ -234,7 +234,7 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_ICK_ID("sci_fck", "sh-sci.1", &mstp_clks[MSTP006]),
 	CLKDEV_ICK_ID("sci_fck", "sh-sci.2", &mstp_clks[MSTP005]),
 
-	CLKDEV_CON_ID("msiof0", &mstp_clks[MSTP002]),
+	CLKDEV_DEV_ID("spi_sh_msiof.0", &mstp_clks[MSTP002]),
 	CLKDEV_CON_ID("sbr0", &mstp_clks[MSTP001]),
 	CLKDEV_DEV_ID("i2c-sh_mobile.0", &mstp_clks[MSTP109]),
 	CLKDEV_CON_ID("icb0", &mstp_clks[MSTP227]),
@@ -242,16 +242,16 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_CON_ID("dacy1", &mstp_clks[MSTP224]),
 	CLKDEV_CON_ID("dacy0", &mstp_clks[MSTP223]),
 	CLKDEV_CON_ID("tsif0", &mstp_clks[MSTP222]),
-	CLKDEV_CON_ID("sdhi0", &mstp_clks[MSTP218]),
-	CLKDEV_CON_ID("mmcif0", &mstp_clks[MSTP217]),
+	CLKDEV_DEV_ID("sh_mobile_sdhi.0", &mstp_clks[MSTP218]),
+	CLKDEV_DEV_ID("sh_mmcif.0", &mstp_clks[MSTP217]),
 	CLKDEV_CON_ID("usbf0", &mstp_clks[MSTP211]),
 	CLKDEV_CON_ID("veu1", &mstp_clks[MSTP207]),
-	CLKDEV_CON_ID("vou0", &mstp_clks[MSTP205]),
+	CLKDEV_DEV_ID("sh-vou.0", &mstp_clks[MSTP205]),
 	CLKDEV_CON_ID("beu0", &mstp_clks[MSTP204]),
-	CLKDEV_CON_ID("ceu0", &mstp_clks[MSTP203]),
+	CLKDEV_DEV_ID("sh_mobile_ceu.0", &mstp_clks[MSTP203]),
 	CLKDEV_CON_ID("veu0", &mstp_clks[MSTP202]),
 	CLKDEV_CON_ID("vpu0", &mstp_clks[MSTP201]),
-	CLKDEV_CON_ID("lcdc0", &mstp_clks[MSTP200]),
+	CLKDEV_DEV_ID("sh_mobile_lcdc_fb.0", &mstp_clks[MSTP200]),
 };
 
 int __init arch_clk_init(void)
diff --git a/arch/sh/kernel/cpu/sh4a/clock-sh7757.c b/arch/sh/kernel/cpu/sh4a/clock-sh7757.c
index 04ab5ae..4cbd9a3 100644
--- a/arch/sh/kernel/cpu/sh4a/clock-sh7757.c
+++ b/arch/sh/kernel/cpu/sh4a/clock-sh7757.c
@@ -131,7 +131,7 @@ static struct clk_lookup lookups[] = {
 
 	CLKDEV_CON_ID("usb_fck", &mstp_clks[MSTP103]),
 	CLKDEV_DEV_ID("renesas_usbhs.0", &mstp_clks[MSTP102]),
-	CLKDEV_CON_ID("mmc0", &mstp_clks[MSTP220]),
+	CLKDEV_DEV_ID("sh_mmcif.0", &mstp_clks[MSTP220]),
 	CLKDEV_CON_ID("rspi2", &mstp_clks[MSTP127]),
 };
 
diff --git a/arch/sh/kernel/cpu/sh4a/clock-sh7785.c b/arch/sh/kernel/cpu/sh4a/clock-sh7785.c
index ab1c58f..6dea6b6 100644
--- a/arch/sh/kernel/cpu/sh4a/clock-sh7785.c
+++ b/arch/sh/kernel/cpu/sh4a/clock-sh7785.c
@@ -143,7 +143,7 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_CON_ID("ssi0_fck", &mstp_clks[MSTP020]),
 	CLKDEV_CON_ID("hac1_fck", &mstp_clks[MSTP017]),
 	CLKDEV_CON_ID("hac0_fck", &mstp_clks[MSTP016]),
-	CLKDEV_CON_ID("mmcif_fck", &mstp_clks[MSTP013]),
+	CLKDEV_DEV_ID("sh_mmcif.0", &mstp_clks[MSTP013]),
 	CLKDEV_CON_ID("flctl_fck", &mstp_clks[MSTP012]),
 
 	CLKDEV_ICK_ID("tmu_fck", "sh_tmu.0", &mstp_clks[MSTP008]),
@@ -157,8 +157,8 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_CON_ID("hspi_fck", &mstp_clks[MSTP002]),
 	CLKDEV_CON_ID("hudi_fck", &mstp_clks[MSTP119]),
 	CLKDEV_CON_ID("ubc0", &mstp_clks[MSTP117]),
-	CLKDEV_CON_ID("dmac_11_6_fck", &mstp_clks[MSTP105]),
-	CLKDEV_CON_ID("dmac_5_0_fck", &mstp_clks[MSTP104]),
+	CLKDEV_DEV_ID("sh-dma-engine.1", &mstp_clks[MSTP105]),
+	CLKDEV_DEV_ID("sh-dma-engine.0", &mstp_clks[MSTP104]),
 	CLKDEV_CON_ID("gdta_fck", &mstp_clks[MSTP100]),
 };
 
diff --git a/arch/sh/kernel/cpu/sh4a/clock-sh7786.c b/arch/sh/kernel/cpu/sh4a/clock-sh7786.c
index 4917094..c7107c5 100644
--- a/arch/sh/kernel/cpu/sh4a/clock-sh7786.c
+++ b/arch/sh/kernel/cpu/sh4a/clock-sh7786.c
@@ -152,8 +152,8 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_CON_ID("ssi0_fck", &mstp_clks[MSTP020]),
 	CLKDEV_CON_ID("hac1_fck", &mstp_clks[MSTP017]),
 	CLKDEV_CON_ID("hac0_fck", &mstp_clks[MSTP016]),
-	CLKDEV_CON_ID("i2c1_fck", &mstp_clks[MSTP015]),
-	CLKDEV_CON_ID("i2c0_fck", &mstp_clks[MSTP014]),
+	CLKDEV_DEV_ID("i2c-sh_mobile.1", &mstp_clks[MSTP015]),
+	CLKDEV_DEV_ID("i2c-sh_mobile.0", &mstp_clks[MSTP014]),
 
 	CLKDEV_ICK_ID("tmu_fck", "sh_tmu.0", &mstp_clks[MSTP008]),
 	CLKDEV_ICK_ID("tmu_fck", "sh_tmu.1", &mstp_clks[MSTP008]),
@@ -175,8 +175,8 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_CON_ID("pcie2_fck", &mstp_clks[MSTP110]),
 	CLKDEV_CON_ID("pcie1_fck", &mstp_clks[MSTP109]),
 	CLKDEV_CON_ID("pcie0_fck", &mstp_clks[MSTP108]),
-	CLKDEV_CON_ID("dmac_11_6_fck", &mstp_clks[MSTP105]),
-	CLKDEV_CON_ID("dmac_5_0_fck", &mstp_clks[MSTP104]),
+	CLKDEV_DEV_ID("sh-dma-engine.1", &mstp_clks[MSTP105]),
+	CLKDEV_DEV_ID("sh-dma-engine.0", &mstp_clks[MSTP104]),
 	CLKDEV_CON_ID("du_fck", &mstp_clks[MSTP103]),
 	CLKDEV_CON_ID("ether_fck", &mstp_clks[MSTP102]),
 };
diff --git a/arch/sh/kernel/cpu/sh4a/clock-shx3.c b/arch/sh/kernel/cpu/sh4a/clock-shx3.c
index 0f11b39..be753ee 100644
--- a/arch/sh/kernel/cpu/sh4a/clock-shx3.c
+++ b/arch/sh/kernel/cpu/sh4a/clock-shx3.c
@@ -132,8 +132,8 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_ICK_ID("tmu_fck", "sh_tmu.5", &mstp_clks[MSTP009]),
 
 	CLKDEV_CON_ID("hudi_fck", &mstp_clks[MSTP119]),
-	CLKDEV_CON_ID("dmac_11_6_fck", &mstp_clks[MSTP105]),
-	CLKDEV_CON_ID("dmac_5_0_fck", &mstp_clks[MSTP104]),
+	CLKDEV_DEV_ID("sh-dma-engine.1", &mstp_clks[MSTP105]),
+	CLKDEV_DEV_ID("sh-dma-engine.0", &mstp_clks[MSTP104]),
 };
 
 int __init arch_clk_init(void)
-- 
1.7.2.5


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] sh: switch various MSTP PM clocks to device-ID look-up
  2012-04-26 21:48 [PATCH] sh: switch various MSTP PM clocks to device-ID look-up Guennadi Liakhovetski
@ 2012-04-27  0:31 ` Paul Mundt
  2012-04-27  7:26 ` Guennadi Liakhovetski
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Paul Mundt @ 2012-04-27  0:31 UTC (permalink / raw)
  To: linux-sh

On Thu, Apr 26, 2012 at 11:48:32PM +0200, Guennadi Liakhovetski wrote:
> Most SH drivers, accessing clocks, used for their devices, directly, have
> been converted to not use connection IDs for clock look-up. The runtime PM
> subsystem does the same. This means, that clock look-up entries for such
> devices with non-NULL connection IDs are useless. This patch converts such
> clock look-up entries to correct device IDs.
> 
This seems more like a failure in runtime PM than an issue with
connection IDs. We use connection IDs particularly in cases where there
is ambiguity, or multiple clocks for the same device, which we want to
toggle at different times. We won't be blindly moving off of connection
IDs where that behaviour is desirable if there is insufficiently granular
infratructure to migrate to.

Likewise having these sorts of arbitrary policy decisions offline is
completely pointless. If you wish to put forward a concrete proposal for
moving off of connection IDs, then send it to the list and let it stand
on its own merit.

> Another thing to note - I didn't convert TMU entries on sh7343 and sh7366. 
> On both these platforms currently there's one tmu clock look-up entry with 
> just a "tmu_fck" connection ID, but platforms themselves register multiple 
> tmu devices. Without datasheets I have no idea, whether some clock entries 
> are missing from the lookup table, or that entry controlls all TMU clocks, 
> or only one (#0?) TMU clock can be switched on and off.
> 
Most TMU clocks are grouped in pairs of 3 or 4, so one MSTP bit will
control channels 0 .. 3, etc.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] sh: switch various MSTP PM clocks to device-ID look-up
  2012-04-26 21:48 [PATCH] sh: switch various MSTP PM clocks to device-ID look-up Guennadi Liakhovetski
  2012-04-27  0:31 ` Paul Mundt
@ 2012-04-27  7:26 ` Guennadi Liakhovetski
  2012-05-24  8:56 ` Guennadi Liakhovetski
  2012-05-24 14:48 ` Paul Mundt
  3 siblings, 0 replies; 5+ messages in thread
From: Guennadi Liakhovetski @ 2012-04-27  7:26 UTC (permalink / raw)
  To: linux-sh

Hi Paul

On Fri, 27 Apr 2012, Paul Mundt wrote:

> On Thu, Apr 26, 2012 at 11:48:32PM +0200, Guennadi Liakhovetski wrote:
> > Most SH drivers, accessing clocks, used for their devices, directly, have
> > been converted to not use connection IDs for clock look-up. The runtime PM
> > subsystem does the same. This means, that clock look-up entries for such
> > devices with non-NULL connection IDs are useless. This patch converts such
> > clock look-up entries to correct device IDs.
> > 
> This seems more like a failure in runtime PM than an issue with
> connection IDs.

This can be discussed, however, this is how it is today: sh-mobile runtime 
PM isn't using clock connection ID to identify PM related clocks. The API 
itself has this ability and OMAP1 is using it.

> We use connection IDs particularly in cases where there
> is ambiguity, or multiple clocks for the same device, which we want to
> toggle at different times.

Of course.

> We won't be blindly moving off of connection
> IDs where that behaviour is desirable if there is insufficiently granular
> infratructure to migrate to.

I'm not proposing that. All the cases, that I modified in this patch only 
have one clock per device, so, there's no ambiguity. And in such simple 
cases it seems logical to simply drop connection IDs. We have several 
cases with multiple clocks per device, like audio, HDMI, maybe some 
others. There we do use connection IDs.

> Likewise having these sorts of arbitrary policy decisions offline is
> completely pointless. If you wish to put forward a concrete proposal for
> moving off of connection IDs, then send it to the list and let it stand
> on its own merit.

As I said, I'm not proposing to move off connection IDs globally, only for 
non-ambiguous cases.

> > Another thing to note - I didn't convert TMU entries on sh7343 and sh7366. 
> > On both these platforms currently there's one tmu clock look-up entry with 
> > just a "tmu_fck" connection ID, but platforms themselves register multiple 
> > tmu devices. Without datasheets I have no idea, whether some clock entries 
> > are missing from the lookup table, or that entry controlls all TMU clocks, 
> > or only one (#0?) TMU clock can be switched on and off.
> > 
> Most TMU clocks are grouped in pairs of 3 or 4, so one MSTP bit will
> control channels 0 .. 3, etc.

Interesting... Indeed, upon a closer look at sh7724

	CLKDEV_ICK_ID("tmu_fck", "sh_tmu.0", &mstp_clks[HWBLK_TMU0]),
	CLKDEV_ICK_ID("tmu_fck", "sh_tmu.1", &mstp_clks[HWBLK_TMU0]),
	CLKDEV_ICK_ID("tmu_fck", "sh_tmu.2", &mstp_clks[HWBLK_TMU0]),

does create 3 lookup entries for one clock with the same connection and 
different device IDs. That's probably also what has to be done for sh7343 
and sh7366.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] sh: switch various MSTP PM clocks to device-ID look-up
  2012-04-26 21:48 [PATCH] sh: switch various MSTP PM clocks to device-ID look-up Guennadi Liakhovetski
  2012-04-27  0:31 ` Paul Mundt
  2012-04-27  7:26 ` Guennadi Liakhovetski
@ 2012-05-24  8:56 ` Guennadi Liakhovetski
  2012-05-24 14:48 ` Paul Mundt
  3 siblings, 0 replies; 5+ messages in thread
From: Guennadi Liakhovetski @ 2012-05-24  8:56 UTC (permalink / raw)
  To: linux-sh

Hi Paul

On Fri, 27 Apr 2012, Paul Mundt wrote:

> On Thu, Apr 26, 2012 at 11:48:32PM +0200, Guennadi Liakhovetski wrote:
> > Most SH drivers, accessing clocks, used for their devices, directly, have
> > been converted to not use connection IDs for clock look-up. The runtime PM
> > subsystem does the same. This means, that clock look-up entries for such
> > devices with non-NULL connection IDs are useless. This patch converts such
> > clock look-up entries to correct device IDs.
> > 
> This seems more like a failure in runtime PM than an issue with
> connection IDs. We use connection IDs particularly in cases where there
> is ambiguity, or multiple clocks for the same device, which we want to
> toggle at different times. We won't be blindly moving off of connection
> IDs where that behaviour is desirable if there is insufficiently granular
> infratructure to migrate to.
> 
> Likewise having these sorts of arbitrary policy decisions offline is
> completely pointless. If you wish to put forward a concrete proposal for
> moving off of connection IDs, then send it to the list and let it stand
> on its own merit.

Sorry for an almost one month long pause. Maybe we could close this thread 
by verifying my current understanding and checking your preferences.

1. The drivers I was looking at: SDHI, MMCIF, MSIOF all do clk_get(dev, 
   "name%d"); where name%d is sdhi%d, mmc%d and msiof%d respectively.
2. Some other drivers, like i2c do just clk_get(dev, NULL);
3. Others yet, e.g., sci do clk_get(dev, name), where name is fixed - it 
   doesn't have the interface number in it. The sci driver uses two clocks 
   with connection IDs "sci_ick" and "sci_fck."

This seems a bit messy to me, but, well, as long as it works - no need to 
change anything there:-) My understanding would be, that a preferred 
scheme would be to either use no connection ID at all or use a fixed one - 
without an index. On the platform side:

1. The runtime PM acquires clocks per clk_get(dev, NULL)
2. Some platforms like sh7724, sh7722 on the sh side, all ARM SoCs use 
   mostly CLKDEV_DEV_ID() and CLKDEV_CON_ID() entries in their clk lookup 
   tables.
3. CLKDEV_ICK_ID() entries are used occasionally, mostly, when more than 
   one clock per device is registered
4. On other platforms, like those, affected by this patch, mainly or 
   exclusively CLKDEV_CON_ID() entries are used.

Here my understanding is, that for clocks, that should be referenced from 
the runtime PM a device reference is compulsory, so, CLKDEV_ICK_ID() or 
CLKDEV_DEV_ID() should be used. For clocks, used by drivers with an 
indexed connection ID, CLKDEV_CON_ID() entries will work too. This means, 
that at least the runtime PM on platforms, touched by this patch, is 
disabled for affected devices, but I personally have no idea how much of a 
problem that is, I don't work with any of them.

So, if this situation is acceptable, no problem with me, we can drop this 
series. If, however, we want to change something in this, would be nice to 
know your opinion. If my this patch-set can be adjusted to fulfill any of 
your wishes in this area - we could try to do that:-)

Thanks
Guennadi

> > Another thing to note - I didn't convert TMU entries on sh7343 and sh7366. 
> > On both these platforms currently there's one tmu clock look-up entry with 
> > just a "tmu_fck" connection ID, but platforms themselves register multiple 
> > tmu devices. Without datasheets I have no idea, whether some clock entries 
> > are missing from the lookup table, or that entry controlls all TMU clocks, 
> > or only one (#0?) TMU clock can be switched on and off.
> > 
> Most TMU clocks are grouped in pairs of 3 or 4, so one MSTP bit will
> control channels 0 .. 3, etc.
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] sh: switch various MSTP PM clocks to device-ID look-up
  2012-04-26 21:48 [PATCH] sh: switch various MSTP PM clocks to device-ID look-up Guennadi Liakhovetski
                   ` (2 preceding siblings ...)
  2012-05-24  8:56 ` Guennadi Liakhovetski
@ 2012-05-24 14:48 ` Paul Mundt
  3 siblings, 0 replies; 5+ messages in thread
From: Paul Mundt @ 2012-05-24 14:48 UTC (permalink / raw)
  To: linux-sh

On Thu, May 24, 2012 at 10:56:32AM +0200, Guennadi Liakhovetski wrote:
> On Fri, 27 Apr 2012, Paul Mundt wrote:
> > On Thu, Apr 26, 2012 at 11:48:32PM +0200, Guennadi Liakhovetski wrote:
> > This seems more like a failure in runtime PM than an issue with
> > connection IDs. We use connection IDs particularly in cases where there
> > is ambiguity, or multiple clocks for the same device, which we want to
> > toggle at different times. We won't be blindly moving off of connection
> > IDs where that behaviour is desirable if there is insufficiently granular
> > infratructure to migrate to.
> 
> Sorry for an almost one month long pause. Maybe we could close this thread 
> by verifying my current understanding and checking your preferences.
> 
No problem, it's better to hash things out in email even if it takes some
cycles rather than burning cycles on patches prematurely.

> 1. The drivers I was looking at: SDHI, MMCIF, MSIOF all do clk_get(dev, 
>    "name%d"); where name%d is sdhi%d, mmc%d and msiof%d respectively.
> 2. Some other drivers, like i2c do just clk_get(dev, NULL);
> 3. Others yet, e.g., sci do clk_get(dev, name), where name is fixed - it 
>    doesn't have the interface number in it. The sci driver uses two clocks 
>    with connection IDs "sci_ick" and "sci_fck."
> 
> This seems a bit messy to me, but, well, as long as it works - no need to 
> change anything there:-) My understanding would be, that a preferred 
> scheme would be to either use no connection ID at all or use a fixed one - 
> without an index. On the platform side:
> 
I agree with you that the first two are messy and doing something about
them is a worthwhile endeavour. I think where we either have a
misunderstanding or a differing of opinions is the 3rd case.

With regards to the lack of an interface number, that's intentional and
by design. All clock lookups using connection IDs factor in the interface
number at CLKDEV_ICK_ID() time, and as you have correctly identified,
this is primarily of value for when we have multiple clocks for one
device.

Perhaps if we use sci as an example you'll see why we went with the
abstraction that way:

	- We have cases where gating of the block is controlled
	  independently of the actual clock that's of any relevance to
	  the driver. 

		- This is especially visible in cases where we have MSTP
		  bits wrapped to function clocks that control block
		  gating but don't actually have anything to do with the
		  clock input otherwise.

	- The main clock driving sh-sci is not sci_fck, but sci_ick (or
	  in the case where it doesn't exist, pclk).

		- While sci_fck exists for off/on control, it has very
		  little other functionality.
		  
	- sci_ick can be arbitrarily defined.

		- At present this is primarily wrapped to the pclk
		  value due to the fact we're using external clock input
		  to drive the block.

		- The intent has always been to support switching over to
		  having the sh-sci block generate its own clock when
		  necessary, once we have a facility in place to register
		  sci_ick from within the driver itself.

		- We have quite a few blocks that follow this model.

So, in summary:

	- sci_fck: module on/off
	- sci_ick: pclk if external, otherwise internally driven

For extra points:

	- Internal generation is a per-port property, so we can have
	  cases where internal generation is used for some ports while
	  the pclk input is used for others (this largely depends on both
	  the baud rate generator and pinmux state).

In these sorts of cases I suppose we have to look at what runtime PM
cares about. I expect that it's only sci_fck that runtime PM requires any
visibility of, while the sci_ick state remains an internal property. I
expect there is some way that we can get the two to co-exist and get the
sort of behaviour that you're after, but it's not obvious that the
present runtime PM approach is sufficiently granular for dealing with
this. clk_get(dev, NULL) certainly doesn't contain enough information
for us to work out whether sci_fck or _ick is needed, and they will both
need to be enabled/disabled at different times and for different reasons.

> Here my understanding is, that for clocks, that should be referenced from 
> the runtime PM a device reference is compulsory, so, CLKDEV_ICK_ID() or 
> CLKDEV_DEV_ID() should be used. For clocks, used by drivers with an 
> indexed connection ID, CLKDEV_CON_ID() entries will work too. This means, 
> that at least the runtime PM on platforms, touched by this patch, is 
> disabled for affected devices, but I personally have no idea how much of a 
> problem that is, I don't work with any of them.
> 
Perhaps part of the problem is that MSTP control really doesn't really
belong in the clock framework to begin with. MSTP control was shoe-horned
in because that's more or less where it fit from an API point of view in
terms of block enable/disable functionality -- obviously pre-dating
runtime PM and others. Perhaps we're at a point where we need to think
about decoupling MSTP control in to a different API and then thinking
about how to tie that in to runtime PM. On the other hand, the clock
framework utilization by runtime PM at the moment matches MSTP control
pretty well, so I'm also not in a hurry to attempt to introduce another
paradigm and create even more busy work (this was already tried and
abandoned with the hwblk API and so on).

Perhaps the simplest solution is simply having some API extension in
runtime PM in which we are able to associate a function clock for control
on a per-device level without relying purely on clk_get(dev, NULL) to do
the right thing -- indeed this is probably the way things are going to
have to go for the common struct clk fiasco, which for reasons
unbeknownst to me favours pre-clkdev clk_get_sys() string lookups from
the dark ages for everything.

> So, if this situation is acceptable, no problem with me, we can drop this 
> series. If, however, we want to change something in this, would be nice to 
> know your opinion. If my this patch-set can be adjusted to fulfill any of 
> your wishes in this area - we could try to do that:-)
> 
I don't actually have any problems with the series as you've posted it, I
just bristled at the notion that connection IDs are bad and are to be
avoided, while in practice they do have an important application and is
something runtime PM needs to figure out how to deal with.

In the case of your patch, I think that especially in the grouped case it
helps to break things out. For example, you've switched dmac_11_6_fck and
dmac_5_0_fck to sh-dma-engine.1 and .0 respectively, while those may not
functionally be the same, unless sh-dma-engine.1 controls all of channels
11 to 6 and sh-dma-engine.0 is assuming control of channels 5 to 0. If
it's the case that each channel is being registered independently (as we
do have with the TMU, PCIe, etc.) then we simply maintain ICK behaviour
mapping to the corresponding shared MSTP bit (not really any different
from shared IRQ utilization that the dmaengine stuff is doing, too).

We also have cases where devices with a -1 id are supported alongside
those with >= 0 ids that wish to use the same clock lookup strategy.
Any of your #2 or #3 case as noted above will handle this, while the #1
case is completely broken (there are many cases where we have clk0 names
to support this behaviour even though we're really dealing with a -1 id
case) and should be ripped out of the tree with prejudice.

If you're interested in doing cleanups in this area, there are a few
things that I'd like to see done:

	- Killing anything that attempts to construct a clock string for
	  lookup by hand.

	- Getting rid of the clk0 names where -1 dev id behaviour is
	  implied and working out an alternate way to connect the lookup
	  to the driver. Some of these will actually be valid dev.0 cases
	  that simply predate the introduction of the . in the device
	  name.

	- Naming consistency. Many clocks that we don't use in any driver
	  have weird combination dev and ick or con id strings -- like
	  hac0/1_fck that need to make up their mind. Once we've
	  got a convention in place it'll be easier to prevent people
	  from deviating too much and introducing more driver lookup
	  mess. A lot of this also predates clkdev lookups.

With those issues addressed I think a lot of the ambiguity will go away
and it should be a lot more readily apparent when we need a con/dev/ick
clock lookup.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2012-05-24 14:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-26 21:48 [PATCH] sh: switch various MSTP PM clocks to device-ID look-up Guennadi Liakhovetski
2012-04-27  0:31 ` Paul Mundt
2012-04-27  7:26 ` Guennadi Liakhovetski
2012-05-24  8:56 ` Guennadi Liakhovetski
2012-05-24 14:48 ` Paul Mundt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.