[RFC] i.MX: drop secondary clocks

[RFC] i.MX: drop secondary clocks

[Sascha Hauer]

Hi All,

On i.MX we currently have grouped clocks, also known as secondary clocks.
These are clocks that get enabled when the primary clock gets enabled.  This
doesn't integrate well into the generic clock framework. Also we often return
the rate of one clock and really enable/disable another clock. I plan to get
rid of these clocks by exposing the individual clocks to the drivers. The
following is meant as an example how this can be done. I have a more complete
series internally but this conflicts with Richards clk_prepare series.

Thanks to the nature of clk_get we can simply clk_get the individual clocks in
the drivers and the current clock implementation will always return the one
with con_id = NULL. Then with the generic clk implementation the drivers will
finally get the individual clocks.

So unless there are objections I will follow up on this after the next merge
window.

Sascha

----------------------------------------------------------------
Sascha Hauer (2):
      clk: Add helper to get/put arrays of clocks
      spi i.MX: do not depend on grouped clocks

 drivers/clk/clkdev.c  |   36 ++++++++++++++++++++++++++++++++++++
 drivers/spi/spi-imx.c |   30 ++++++++++++++++++++----------
 include/linux/clk.h   |    3 +++
 3 files changed, 59 insertions(+), 10 deletions(-)

[PATCH 1/2] clk: Add helper to get/put arrays of clocks

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/clk/clkdev.c |   36 ++++++++++++++++++++++++++++++++++++
 include/linux/clk.h  |    3 +++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 6db161f..c661395 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -89,6 +89,42 @@ void clk_put(struct clk *clk)
 }
 EXPORT_SYMBOL(clk_put);
 
+int clk_get_array(struct device *dev, struct clk **clks, const char **names,
+		int numclks, bool msg)
+{
+	int i, ret;
+
+	for (i = 0; i < numclks; i++) {
+		clks[i] = clk_get(dev, names[i]);
+		if (IS_ERR(clks[i])) {
+			if (msg)
+				dev_err(dev, "getting clock \"%s\" failed with %d\n",
+						names[i] ? names[i] : "",
+						PTR_ERR(clks[i]));
+			ret = PTR_ERR(clks[i]);
+			goto err;
+		}
+	}
+
+	return 0;
+err:
+	while (i > 0) {
+		i--;
+		clk_put(clks[i]);
+	}
+	return ret;
+}
+EXPORT_SYMBOL(clk_get_array);
+
+void clk_put_array(struct clk **clks, int numclks)
+{
+	int i;
+
+	for (i = 0; i < numclks; i++)
+		clk_put(clks[i]);
+}
+EXPORT_SYMBOL(clk_put_array);
+
 void clkdev_add(struct clk_lookup *cl)
 {
 	mutex_lock(&clocks_mutex);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index b9d46fa..1584cbd 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -148,6 +148,9 @@ unsigned long clk_get_rate(struct clk *clk);
  */
 void clk_put(struct clk *clk);
 
+int clk_get_array(struct device *dev, struct clk **clks, const char **names,
+		int numclks, bool msg);
+void clk_put_array(struct clk **clks, int numclks);
 
 /*
  * The remaining APIs are optional for machine class support.
-- 
1.7.9.1

[PATCH 2/2] spi i.MX: do not depend on grouped clocks

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/spi/spi-imx.c |   30 ++++++++++++++++++++----------
 1 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index c6e697f..dc32976 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -79,13 +79,20 @@ struct spi_imx_devtype_data {
 	enum spi_imx_devtype devtype;
 };
 
+enum {
+	CLK_PER,
+	CLK_IPG,
+};
+static const char *clknames[] = {"per", "ipg"};
+#define NUM_CLOCKS ARRAY_SIZE(clknames)
+
 struct spi_imx_data {
 	struct spi_bitbang bitbang;
 
 	struct completion xfer_done;
 	void *base;
 	int irq;
-	struct clk *clk;
+	struct clk *clk[NUM_CLOCKS];
 	unsigned long spi_clk;
 
 	unsigned int count;
@@ -846,15 +853,16 @@ static int __devinit spi_imx_probe(struct platform_device *pdev)
 		goto out_iounmap;
 	}
 
-	spi_imx->clk = clk_get(&pdev->dev, NULL);
-	if (IS_ERR(spi_imx->clk)) {
+	ret = clk_get_array(&pdev->dev, spi_imx->clk, clknames, NUM_CLOCKS, 1);
+	if (ret) {
 		dev_err(&pdev->dev, "unable to get clock\n");
-		ret = PTR_ERR(spi_imx->clk);
 		goto out_free_irq;
 	}
 
-	clk_enable(spi_imx->clk);
-	spi_imx->spi_clk = clk_get_rate(spi_imx->clk);
+	clk_prepare_enable(spi_imx->clk[CLK_IPG]);
+	clk_prepare_enable(spi_imx->clk[CLK_PER]);
+
+	spi_imx->spi_clk = clk_get_rate(spi_imx->clk[CLK_PER]);
 
 	spi_imx->devtype_data->reset(spi_imx);
 
@@ -872,8 +880,9 @@ static int __devinit spi_imx_probe(struct platform_device *pdev)
 	return ret;
 
 out_clk_put:
-	clk_disable(spi_imx->clk);
-	clk_put(spi_imx->clk);
+	clk_disable_unprepare(spi_imx->clk[CLK_PER]);
+	clk_disable_unprepare(spi_imx->clk[CLK_IPG]);
+	clk_put_array(spi_imx->clk, NUM_CLOCKS);
 out_free_irq:
 	free_irq(spi_imx->irq, spi_imx);
 out_iounmap:
@@ -901,8 +910,9 @@ static int __devexit spi_imx_remove(struct platform_device *pdev)
 	spi_bitbang_stop(&spi_imx->bitbang);
 
 	writel(0, spi_imx->base + MXC_CSPICTRL);
-	clk_disable(spi_imx->clk);
-	clk_put(spi_imx->clk);
+	clk_disable_unprepare(spi_imx->clk[CLK_PER]);
+	clk_disable_unprepare(spi_imx->clk[CLK_IPG]);
+	clk_put_array(spi_imx->clk, NUM_CLOCKS);
 	free_irq(spi_imx->irq, spi_imx);
 	iounmap(spi_imx->base);
 
-- 
1.7.9.1

[RFC] i.MX: drop secondary clocks

[Shawn Guo]

On Thu, Mar 08, 2012 at 10:42:27PM +0100, Sascha Hauer wrote:
> Hi All,
> 
> On i.MX we currently have grouped clocks, also known as secondary clocks.
> These are clocks that get enabled when the primary clock gets enabled.  This
> doesn't integrate well into the generic clock framework. Also we often return
> the rate of one clock and really enable/disable another clock. I plan to get
> rid of these clocks by exposing the individual clocks to the drivers. The
> following is meant as an example how this can be done. I have a more complete
> series internally but this conflicts with Richards clk_prepare series.
> 
> Thanks to the nature of clk_get we can simply clk_get the individual clocks in
> the drivers and the current clock implementation will always return the one
> with con_id = NULL. Then with the generic clk implementation the drivers will
> finally get the individual clocks.
> 
> So unless there are objections I will follow up on this after the next merge
> window.
> 
So glad to see you are getting rid of this nasty imx secondary clock.
I agree this is the way to go.

-- 
Regards,
Shawn

[RFC] i.MX: drop secondary clocks

[Lothar Waßmann]

Hi,

Sascha Hauer writes:
> Hi All,
> 
> On i.MX we currently have grouped clocks, also known as secondary clocks.
> These are clocks that get enabled when the primary clock gets enabled.  This
> doesn't integrate well into the generic clock framework. Also we often return
> the rate of one clock and really enable/disable another clock. I plan to get
> rid of these clocks by exposing the individual clocks to the drivers. The
> following is meant as an example how this can be done. I have a more complete
> series internally but this conflicts with Richards clk_prepare series.
> 
> Thanks to the nature of clk_get we can simply clk_get the individual clocks in
> the drivers and the current clock implementation will always return the one
> with con_id = NULL. Then with the generic clk implementation the drivers will
> finally get the individual clocks.
> 
> So unless there are objections I will follow up on this after the next merge
> window.
> 
I think that's a bad idea! Drivers should not have to know intimate
details about the clock logic of the CPU core they are running on!

They should simply request a clock for the unit they operate on and
the clock core code should figure out which other clocks need to be
enabled on a specific machine to get the unit working.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[RFC] i.MX: drop secondary clocks

[Sascha Hauer]

On Fri, Mar 09, 2012 at 10:32:30AM +0100, Lothar Wa?mann wrote:
> Hi,
> 
> Sascha Hauer writes:
> > Hi All,
> > 
> > On i.MX we currently have grouped clocks, also known as secondary clocks.
> > These are clocks that get enabled when the primary clock gets enabled.  This
> > doesn't integrate well into the generic clock framework. Also we often return
> > the rate of one clock and really enable/disable another clock. I plan to get
> > rid of these clocks by exposing the individual clocks to the drivers. The
> > following is meant as an example how this can be done. I have a more complete
> > series internally but this conflicts with Richards clk_prepare series.
> > 
> > Thanks to the nature of clk_get we can simply clk_get the individual clocks in
> > the drivers and the current clock implementation will always return the one
> > with con_id = NULL. Then with the generic clk implementation the drivers will
> > finally get the individual clocks.
> > 
> > So unless there are objections I will follow up on this after the next merge
> > window.
> > 
> I think that's a bad idea! Drivers should not have to know intimate
> details about the clock logic of the CPU core they are running on!

They don't have to. There really are different clocks connected to the
devices. If for example you have a look eSDHC Block diagram in the
reference Manual  you'll see that this has three clock inputs. All
we do is to tell the driver about these three clocks which are device
specific and not SoC specific. Which of these clocks are actually
gatetable varies from SoC to SoC. Also on some SoCs two of the three
clocks may be actually the same.

> 
> They should simply request a clock for the unit they operate on and
> the clock core code should figure out which other clocks need to be
> enabled on a specific machine to get the unit working.

Yes, that's what we want to do, except that devices do not need 'a'
clock but several. And the clock core figures out which other clocks
have to be activated, these are the parent clocks.

I also don't really like exposing this to the drivers, but it's a clean
way to do so. A framebuffer driver needs a pixel clock which must be
enabled when the display is turned on. It may also have a register clock
which has to be turned on when we want to access registers. These are
two different clocks and the driver can make use of this (not that we
currently do though...)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC] i.MX: drop secondary clocks

[Lothar Waßmann]

Hi,

Sascha Hauer writes:
> On Fri, Mar 09, 2012 at 10:32:30AM +0100, Lothar Wa?mann wrote:
> > Hi,
> > 
> > Sascha Hauer writes:
> > > Hi All,
> > > 
[...]
> > I think that's a bad idea! Drivers should not have to know intimate
> > details about the clock logic of the CPU core they are running on!
> 
> They don't have to. There really are different clocks connected to the
> devices. If for example you have a look eSDHC Block diagram in the
> reference Manual  you'll see that this has three clock inputs. All
> we do is to tell the driver about these three clocks which are device
> specific and not SoC specific. Which of these clocks are actually
> gatetable varies from SoC to SoC. Also on some SoCs two of the three
> clocks may be actually the same.
> 
At least this should be done in a way that is independent from the
name of the clock on a specific SoC, but is related to the function of
the clock for the driver. Thus instead of e.g. CLK_IPG and CLK_PER use
CLK_REG and CLK_BITRATE.

> > They should simply request a clock for the unit they operate on and
> > the clock core code should figure out which other clocks need to be
> > enabled on a specific machine to get the unit working.
> 
> Yes, that's what we want to do, except that devices do not need 'a'
> clock but several. And the clock core figures out which other clocks
> have to be activated, these are the parent clocks.
> 
What about devices which have multiple 'parent' clocks that are
completely unrelated wrt. each other? In the current clock system you
can model this by having secondary clocks.
How would you represent the current structure on i.MX53:
uart_ipg_clk <- aips_tz1_clk <-+- ahb_clk
                               +-> ahb_max_clk
in the new model?
Should every driver have to request the ahb_max_clk in addition to the
uart_ipg_clk?

What, if you have a device on a number of SoCs with a certain set of
clocks, but on a later SoC another clock is required for the device?
You would not only have to change the driver to add the new clock but
also would have to add that clock as a dummy clock in the core clock
code for all previous SoCs that did not provide that clock.

> I also don't really like exposing this to the drivers, but it's a clean
> way to do so. A framebuffer driver needs a pixel clock which must be
> enabled when the display is turned on. It may also have a register clock
> which has to be turned on when we want to access registers. These are
> two different clocks and the driver can make use of this (not that we
> currently do though...)
> 
But we could do so without any problem.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[RFC] i.MX: drop secondary clocks

[Richard Zhao]

On Fri, Mar 09, 2012 at 12:47:58PM +0100, Lothar Wa?mann wrote:
> Hi,
> 
> Sascha Hauer writes:
> > On Fri, Mar 09, 2012 at 10:32:30AM +0100, Lothar Wa?mann wrote:
> > > Hi,
> > > 
> > > Sascha Hauer writes:
> > > > Hi All,
> > > > 
> [...]
> > > I think that's a bad idea! Drivers should not have to know intimate
> > > details about the clock logic of the CPU core they are running on!
> > 
> > They don't have to. There really are different clocks connected to the
> > devices. If for example you have a look eSDHC Block diagram in the
> > reference Manual  you'll see that this has three clock inputs. All
> > we do is to tell the driver about these three clocks which are device
> > specific and not SoC specific. Which of these clocks are actually
> > gatetable varies from SoC to SoC. Also on some SoCs two of the three
> > clocks may be actually the same.
> > 
> At least this should be done in a way that is independent from the
> name of the clock on a specific SoC, but is related to the function of
> the clock for the driver. Thus instead of e.g. CLK_IPG and CLK_PER use
> CLK_REG and CLK_BITRATE.
> 
> > > They should simply request a clock for the unit they operate on and
> > > the clock core code should figure out which other clocks need to be
> > > enabled on a specific machine to get the unit working.
> > 
> > Yes, that's what we want to do, except that devices do not need 'a'
> > clock but several. And the clock core figures out which other clocks
> > have to be activated, these are the parent clocks.
> > 
> What about devices which have multiple 'parent' clocks that are
> completely unrelated wrt. each other? In the current clock system you
> can model this by having secondary clocks.
> How would you represent the current structure on i.MX53:
> uart_ipg_clk <- aips_tz1_clk <-+- ahb_clk
>                                +-> ahb_max_clk
> in the new model?
> Should every driver have to request the ahb_max_clk in addition to the
> uart_ipg_clk?
Right. binding bus clock and device core clock together is only one of the
use cases of secondary clock. Sascha, I remember we ever talked about it?
Back to the above example, it's caused by bus topology. It needs other
code, maybe busfreq, to support it, and leaving clock code pure.

Thanks
Richard
> 
> What, if you have a device on a number of SoCs with a certain set of
> clocks, but on a later SoC another clock is required for the device?
> You would not only have to change the driver to add the new clock but
> also would have to add that clock as a dummy clock in the core clock
> code for all previous SoCs that did not provide that clock.
> 
> > I also don't really like exposing this to the drivers, but it's a clean
> > way to do so. A framebuffer driver needs a pixel clock which must be
> > enabled when the display is turned on. It may also have a register clock
> > which has to be turned on when we want to access registers. These are
> > two different clocks and the driver can make use of this (not that we
> > currently do though...)
> > 
> But we could do so without any problem.
> 
> 
> Lothar Wa?mann
> -- 
> ___________________________________________________________
> 
> Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
> Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
> Gesch?ftsf?hrer: Matthias Kaussen
> Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
> 
> www.karo-electronics.de | info at karo-electronics.de
> ___________________________________________________________

[RFC] i.MX: drop secondary clocks

[Sascha Hauer]

On Fri, Mar 09, 2012 at 12:47:58PM +0100, Lothar Wa?mann wrote:
> Hi,
> 
> Sascha Hauer writes:
> > On Fri, Mar 09, 2012 at 10:32:30AM +0100, Lothar Wa?mann wrote:
> > > Hi,
> > > 
> > > Sascha Hauer writes:
> > > > Hi All,
> > > > 
> [...]
> > > I think that's a bad idea! Drivers should not have to know intimate
> > > details about the clock logic of the CPU core they are running on!
> > 
> > They don't have to. There really are different clocks connected to the
> > devices. If for example you have a look eSDHC Block diagram in the
> > reference Manual  you'll see that this has three clock inputs. All
> > we do is to tell the driver about these three clocks which are device
> > specific and not SoC specific. Which of these clocks are actually
> > gatetable varies from SoC to SoC. Also on some SoCs two of the three
> > clocks may be actually the same.
> > 
> At least this should be done in a way that is independent from the
> name of the clock on a specific SoC, but is related to the function of
> the clock for the driver. Thus instead of e.g. CLK_IPG and CLK_PER use
> CLK_REG and CLK_BITRATE.

'per', 'ipg' and 'ahb' seems to be a common pattern with all i.MXs. Only
ahb is sometimes refered to as hclk. Do you also have a name for
ahb/hclk? I don't insist on specific names, in the end they are just
names and the pattern I used at least seems mostly consistent.

> 
> > > They should simply request a clock for the unit they operate on and
> > > the clock core code should figure out which other clocks need to be
> > > enabled on a specific machine to get the unit working.
> > 
> > Yes, that's what we want to do, except that devices do not need 'a'
> > clock but several. And the clock core figures out which other clocks
> > have to be activated, these are the parent clocks.
> > 
> What about devices which have multiple 'parent' clocks that are
> completely unrelated wrt. each other? In the current clock system you
> can model this by having secondary clocks.
> How would you represent the current structure on i.MX53:
> uart_ipg_clk <- aips_tz1_clk <-+- ahb_clk
>                                +-> ahb_max_clk
> in the new model?
> Should every driver have to request the ahb_max_clk in addition to the
> uart_ipg_clk?

No.

The dependeny to the aips_tz1_clk comes in because the UART is behind
the AIPS bridge. That's a dependency on bus topology level but not on clock
topology level. If we really have to handle this then on bus level. The
i.MX5 devicetree already shows this bus topology, it should be doable
to hook drivers on fsl,aips-bus which handle the clock.

Unfortunately the documentation where these clocks are really used goes
to zero and the FSL sourcecode is rather confusing in this area. I think
we will learn more about it once we have something in the generic clock
framework which disables the unused clocks.

> 
> What, if you have a device on a number of SoCs with a certain set of
> clocks, but on a later SoC another clock is required for the device?

I assume that any given device has a set of clocks as inputs which
should get device specific names. It's the job of the clock tree
to know which SoC clocks map to which device clocks. If now a newer
UART block has an additional clock, then we would depending on the
device id (fsl,imx7-uart) know that it requires the additional clock
and request it.

> You would not only have to change the driver to add the new clock but
> also would have to add that clock as a dummy clock in the core clock
> code for all previous SoCs that did not provide that clock.
> 
> > I also don't really like exposing this to the drivers, but it's a clean
> > way to do so. A framebuffer driver needs a pixel clock which must be
> > enabled when the display is turned on. It may also have a register clock
> > which has to be turned on when we want to access registers. These are
> > two different clocks and the driver can make use of this (not that we
> > currently do though...)
> > 
> But we could do so without any problem.

Not without exposing the individual clocks to the devices.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC] i.MX: drop secondary clocks

[Lothar Waßmann]

Hi,

Sascha Hauer writes:
> On Fri, Mar 09, 2012 at 12:47:58PM +0100, Lothar Wa?mann wrote:
> > Hi,
> > 
> > Sascha Hauer writes:
> > > On Fri, Mar 09, 2012 at 10:32:30AM +0100, Lothar Wa?mann wrote:
> > > > Hi,
> > > > 
> > > > Sascha Hauer writes:
> > > > > Hi All,
> > > > > 
> > [...]
> > > > I think that's a bad idea! Drivers should not have to know intimate
> > > > details about the clock logic of the CPU core they are running on!
> > > 
> > > They don't have to. There really are different clocks connected to the
> > > devices. If for example you have a look eSDHC Block diagram in the
> > > reference Manual  you'll see that this has three clock inputs. All
> > > we do is to tell the driver about these three clocks which are device
> > > specific and not SoC specific. Which of these clocks are actually
> > > gatetable varies from SoC to SoC. Also on some SoCs two of the three
> > > clocks may be actually the same.
> > > 
> > At least this should be done in a way that is independent from the
> > name of the clock on a specific SoC, but is related to the function of
> > the clock for the driver. Thus instead of e.g. CLK_IPG and CLK_PER use
> > CLK_REG and CLK_BITRATE.
> 
> 'per', 'ipg' and 'ahb' seems to be a common pattern with all i.MXs. Only
> ahb is sometimes refered to as hclk. Do you also have a name for
> ahb/hclk? I don't insist on specific names, in the end they are just
> names and the pattern I used at least seems mostly consistent.
> 
CLK_BUS?

> I assume that any given device has a set of clocks as inputs which
> should get device specific names. It's the job of the clock tree
> to know which SoC clocks map to which device clocks. If now a newer
> UART block has an additional clock, then we would depending on the
> device id (fsl,imx7-uart) know that it requires the additional clock
> and request it.
> 
It's also not the business of a device driver to know any details
about clock dependencies on a specific SoC.
We probably really need a bus layer that can hide the details of the
bus architecture and their corresponding clocks from the device
drivers.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[RFC] i.MX: drop secondary clocks

[Sascha Hauer]

On Fri, Mar 09, 2012 at 03:44:31PM +0100, Lothar Wa?mann wrote:
> Hi,
> 
> Sascha Hauer writes:
> > On Fri, Mar 09, 2012 at 12:47:58PM +0100, Lothar Wa?mann wrote:
> > > Hi,
> > > 
> > > Sascha Hauer writes:
> > > > On Fri, Mar 09, 2012 at 10:32:30AM +0100, Lothar Wa?mann wrote:
> > > > > Hi,
> > > > > 
> > > > > Sascha Hauer writes:
> > > > > > Hi All,
> > > > > > 
> > > [...]
> > > > > I think that's a bad idea! Drivers should not have to know intimate
> > > > > details about the clock logic of the CPU core they are running on!
> > > > 
> > > > They don't have to. There really are different clocks connected to the
> > > > devices. If for example you have a look eSDHC Block diagram in the
> > > > reference Manual  you'll see that this has three clock inputs. All
> > > > we do is to tell the driver about these three clocks which are device
> > > > specific and not SoC specific. Which of these clocks are actually
> > > > gatetable varies from SoC to SoC. Also on some SoCs two of the three
> > > > clocks may be actually the same.
> > > > 
> > > At least this should be done in a way that is independent from the
> > > name of the clock on a specific SoC, but is related to the function of
> > > the clock for the driver. Thus instead of e.g. CLK_IPG and CLK_PER use
> > > CLK_REG and CLK_BITRATE.
> > 
> > 'per', 'ipg' and 'ahb' seems to be a common pattern with all i.MXs. Only
> > ahb is sometimes refered to as hclk. Do you also have a name for
> > ahb/hclk? I don't insist on specific names, in the end they are just
> > names and the pattern I used at least seems mostly consistent.
> > 
> CLK_BUS?
> 
> > I assume that any given device has a set of clocks as inputs which
> > should get device specific names. It's the job of the clock tree
> > to know which SoC clocks map to which device clocks. If now a newer
> > UART block has an additional clock, then we would depending on the
> > device id (fsl,imx7-uart) know that it requires the additional clock
> > and request it.
> > 
> It's also not the business of a device driver to know any details
> about clock dependencies on a specific SoC.
> We probably really need a bus layer that can hide the details of the
> bus architecture and their corresponding clocks from the device
> drivers.

I agree there's room for improvements, but I don't want to make that a
dependency for the generic clock support.

Does anyone know which clocks are really worth to be turned off for
powermanement? I have no idea whether we really save power when we for
example turn off aips_tz1_clk.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |