[PATCH] tmio_mmc: Optionally support using platform clock

[PATCH] tmio_mmc: Optionally support using platform clock

[Guennadi Liakhovetski]

If the platform device has a clock associated with the tmio-mmc device, 
use it.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <damm@opensource.se>
---

Depends (at least logically) on 
http://marc.info/?l=linux-kernel&m=124782904228865&w=2

Sorry if I forgot anyone who volunteered to help out with MMC patches.

 drivers/mmc/host/tmio_mmc.c |   21 +++++++++++++++++++++
 drivers/mmc/host/tmio_mmc.h |    3 +++
 include/linux/mfd/tmio.h    |    1 +
 3 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
index c246191..2a01572 100644
--- a/drivers/mmc/host/tmio_mmc.c
+++ b/drivers/mmc/host/tmio_mmc.c
@@ -32,6 +32,7 @@
 #include <linux/mmc/host.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/tmio.h>
+#include <linux/clk.h>
 
 #include "tmio_mmc.h"
 
@@ -57,10 +58,18 @@ static void tmio_mmc_clk_stop(struct tmio_mmc_host *host)
 	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~0x0100 &
 		sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
 	msleep(10);
+	if (!IS_ERR(host->clk) && host->clk_is_running) {
+		clk_disable(host->clk);
+		host->clk_is_running = false;
+	}
 }
 
 static void tmio_mmc_clk_start(struct tmio_mmc_host *host)
 {
+	if (!IS_ERR(host->clk) && !host->clk_is_running &&
+	    !clk_enable(host->clk))
+		host->clk_is_running = true;
+
 	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, 0x0100 |
 		sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
 	msleep(10);
@@ -567,6 +576,11 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 			goto unmap_cnf;
 	}
 
+	host->clk = clk_get(&dev->dev, pdata->clk_name);
+
+	if (!IS_ERR(host->clk) && !clk_enable(host->clk))
+		host->clk_is_running = true;
+
 	/* Enable the MMC/SD Control registers */
 	sd_config_write16(host, CNF_CMD, SDCREN);
 	sd_config_write32(host, CNF_CTL_BASE,
@@ -608,6 +622,11 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	return 0;
 
 unmap_cnf:
+	if (!IS_ERR(host->clk)) {
+		clk_disable(host->clk);
+		host->clk_is_running = false;
+		clk_put(host->clk);
+	}
 	if (host->cnf)
 		iounmap(host->cnf);
 unmap_ctl:
@@ -632,6 +651,8 @@ static int __devexit tmio_mmc_remove(struct platform_device *dev)
 		if (host->cnf)
 			iounmap(host->cnf);
 		mmc_free_host(mmc);
+		if (!IS_ERR(host->clk))
+			clk_put(host->clk);
 	}
 
 	return 0;
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 45f06aa..a76d237 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -108,6 +108,7 @@
 		sd_ctrl_write32((host), CTL_STATUS, mask); \
 	} while (0)
 
+struct clk;
 
 struct tmio_mmc_host {
 	void __iomem *cnf;
@@ -117,6 +118,8 @@ struct tmio_mmc_host {
 	struct mmc_request      *mrq;
 	struct mmc_data         *data;
 	struct mmc_host         *mmc;
+	struct clk		*clk;
+	bool			clk_is_running;
 	int                     irq;
 
 	/* pio related stuff */
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index 6b9c5d0..e405895 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -23,6 +23,7 @@
  */
 struct tmio_mmc_data {
 	const unsigned int		hclk;
+	const char			*clk_name;
 };
 
 /*
-- 
1.6.2.4

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[Matt Fleming]

On Tue, Jul 28, 2009 at 09:51:07AM +0200, Guennadi Liakhovetski wrote:
> If the platform device has a clock associated with the tmio-mmc device, 
> use it.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <damm@opensource.se>
> ---
> 

Looks fine to me, though it's probably best to wait for an ACK from Ian.

Reviewed-by: Matt Fleming <matt@console-pimps.org>

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[Ian Molton]

Guennadi Liakhovetski wrote:
> If the platform device has a clock associated with the tmio-mmc device, 
> use it.

As RMK has stated manny times before now, you cannot pass struct clks to 
devices like this, they should be claimed using the clk api.

The problem for tmio-mmc is that the struct clk is tied to the 
architecture code, so at present we cant create new clk providers. What 
we need is for the clk api to be divorced from the arch code so that the 
TMIO MFD driver can create its own clocks that can be claimed by its 
children.

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[Ian Molton]

Guennadi Liakhovetski wrote:
> If the platform device has a clock associated with the tmio-mmc device, 
> use it.

Sorry, I misread there.

I'm still not sure what to to about this though because we seem to be 
collecting numerous ways of passing clocks to this driver, this is the 
fourth, by my counting.

the clock API could cope with all of them by simply allowing the driver 
to claim CLK_MMC (or such) from its parent, except that it cant cope 
with both the platform and MFD code providing clocks. The parent could 
be either the TMIO MFD core (for TMIO MFDs) or the CPU/SoC whatever, it 
woudlnt matter.

In any case, still no, as with all the other TMIO clock code patches. 
This needs to be done properly.

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[Guennadi Liakhovetski]

On Tue, 28 Jul 2009, Ian Molton wrote:

> Guennadi Liakhovetski wrote:
> > If the platform device has a clock associated with the tmio-mmc device, use
> > it.
> 
> Sorry, I misread there.
> 
> I'm still not sure what to to about this though because we seem to be
> collecting numerous ways of passing clocks to this driver, this is the fourth,
> by my counting.
> 
> the clock API could cope with all of them by simply allowing the driver to
> claim CLK_MMC (or such) from its parent, except that it cant cope with both
> the platform and MFD code providing clocks. The parent could be either the
> TMIO MFD core (for TMIO MFDs) or the CPU/SoC whatever, it woudlnt matter.
> 
> In any case, still no, as with all the other TMIO clock code patches. This
> needs to be done properly.

Hi Ian

Thanks for the review.

I understand your concerns. Of course, the _proper_ solution would be to 
implement an architecture-independent clock API, something like what 
clocklib was trying to do. So, yes, if clocklib were in place now, I 
certainly would have used it.

I searched for those clocklib submission attempts (Dmitry added to CC). 
Last one I can find (maybe I missed some) is from July 2008 - more than a 
year ago. So, looks like our options currently are:

1. wait for new submissions of clocklib - if any are planned
2. develop a completely new arch-independent clock API approach
3. take over patches from Dmitry and bring them to a state acceptable for 
mainline

(any more I missed)

No idea about 1, hopefully, Dmitry can tell if he has any near future 
plans to resubmit his patches.

I personally don't have free (as in beer) time to work on 2 or 3. Anyone?

So, unless we hear, that one of the 1-3 is going to happen real soon now, 
I think, it would be unfair to leave SuperH without a proper MMC driver in 
the mainline for an indefinite time, when one can be trivially achieved.

As for your debugging concern, we could allow configuration-less operation 
only on SuperH in tmio_mmc, how about that?

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[Ian Molton]

Guennadi Liakhovetski wrote:

> Hi Ian

Hi!

> Thanks for the review.

No problem - I hope you dont feel that I am picking on you in particular 
here, this problem is not isolated to your patches.

> I understand your concerns. Of course, the _proper_ solution would be to 
> implement an architecture-independent clock API, something like what 
> clocklib was trying to do. So, yes, if clocklib were in place now, I 
> certainly would have used it.

Which is of course a chicken/egg situation. I dealt with this a number 
of times during the e-series development, and every time, integration 
with mainline has gone more swiftly and with better quality, when the 
job was done properly to begin with.

> I searched for those clocklib submission attempts (Dmitry added to CC). 

Also adding RMK to the CC:

> Last one I can find (maybe I missed some) is from July 2008 - more than a 
> year ago. So, looks like our options currently are:
> 
> 1. wait for new submissions of clocklib - if any are planned

Dmitry: whats the current status of your clocklib work?

> 2. develop a completely new arch-independent clock API approach

No chance.

> 3. take over patches from Dmitry and bring them to a state acceptable for 
> mainline

Take over / collaborate with. I'll happily help people push these patches.

> I personally don't have free (as in beer) time to work on 2 or 3. Anyone?

If I can find out what the current state of this stuff is, I'll help get 
it working - I can test / update all the TC6x and T7x MFD devices (and 
probably asic3 too)

> So, unless we hear, that one of the 1-3 is going to happen real soon now, 
> I think, it would be unfair to leave SuperH without a proper MMC driver in 
> the mainline for an indefinite time, when one can be trivially achieved.

Yes, because listening makes code write itself...

If the changes are so trivial, it wont hurt to maintain them as a patch. 
certainly tmio-mmc doesnt change very rapidly so the patch wont need 
regenerating much. You already have this patchset.

So since thats taken care of, we're all now free to work on updating 
the clock API. And once thats done, you can drop your patch and add one 
line to create / alias the clock in your superH platform code.

> As for your debugging concern, we could allow configuration-less operation 
> only on SuperH in tmio_mmc, how about that?

No. The correct way to support optional parts of the hardware is to 
simply not access them when they are not there.

As I said, I'll happily take a patch to abstract power control for the 
tmio-mmc driver. This should remove most of the config area accesses 
from the driver (and shrink your patch!). The remainder are all about 
clock config, and will disappear once we sort the clock API.

Mainline isn't about 'fair' its about 'right'.

-Ian

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[pHilipp Zabel]

On Tue, Jul 28, 2009 at 9:51 AM, Guennadi
Liakhovetski<g.liakhovetski@gmx.de> wrote:
> If the platform device has a clock associated with the tmio-mmc device,
> use it.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <damm@opensource.se>
> ---
>
> Depends (at least logically) on
> http://marc.info/?l=linux-kernel&m=124782904228865&w=2
>
> Sorry if I forgot anyone who volunteered to help out with MMC patches.
>
>  drivers/mmc/host/tmio_mmc.c |   21 +++++++++++++++++++++
>  drivers/mmc/host/tmio_mmc.h |    3 +++
>  include/linux/mfd/tmio.h    |    1 +
>  3 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
> index c246191..2a01572 100644
> --- a/drivers/mmc/host/tmio_mmc.c
> +++ b/drivers/mmc/host/tmio_mmc.c
> @@ -32,6 +32,7 @@
>  #include <linux/mmc/host.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/tmio.h>
> +#include <linux/clk.h>
>
>  #include "tmio_mmc.h"
>
> @@ -57,10 +58,18 @@ static void tmio_mmc_clk_stop(struct tmio_mmc_host *host)
>        sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~0x0100 &
>                sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
>        msleep(10);
> +       if (!IS_ERR(host->clk) && host->clk_is_running) {
> +               clk_disable(host->clk);
> +               host->clk_is_running = false;
> +       }
>  }
>
>  static void tmio_mmc_clk_start(struct tmio_mmc_host *host)
>  {
> +       if (!IS_ERR(host->clk) && !host->clk_is_running &&
> +           !clk_enable(host->clk))
> +               host->clk_is_running = true;
> +
>        sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, 0x0100 |
>                sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
>        msleep(10);
> @@ -567,6 +576,11 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
>                        goto unmap_cnf;
>        }
>
> +       host->clk = clk_get(&dev->dev, pdata->clk_name);

Instead of pdata->clk_name, this should be either NULL or better
"HCLK" (to differentiate from CLK32, not sure if we want to be able to
toggle that, too, in the future).
Passing the clock consumer ID via pdata is just wrong and shouldn't be
needed with clkdev.

> +
> +       if (!IS_ERR(host->clk) && !clk_enable(host->clk))
> +               host->clk_is_running = true;
> +
>        /* Enable the MMC/SD Control registers */
>        sd_config_write16(host, CNF_CMD, SDCREN);
>        sd_config_write32(host, CNF_CTL_BASE,
> @@ -608,6 +622,11 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
>        return 0;
>
>  unmap_cnf:
> +       if (!IS_ERR(host->clk)) {
> +               clk_disable(host->clk);
> +               host->clk_is_running = false;
> +               clk_put(host->clk);
> +       }
>        if (host->cnf)
>                iounmap(host->cnf);
>  unmap_ctl:
> @@ -632,6 +651,8 @@ static int __devexit tmio_mmc_remove(struct platform_device *dev)
>                if (host->cnf)
>                        iounmap(host->cnf);
>                mmc_free_host(mmc);
> +               if (!IS_ERR(host->clk))
> +                       clk_put(host->clk);
>        }
>
>        return 0;
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index 45f06aa..a76d237 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -108,6 +108,7 @@
>                sd_ctrl_write32((host), CTL_STATUS, mask); \
>        } while (0)
>
> +struct clk;
>
>  struct tmio_mmc_host {
>        void __iomem *cnf;
> @@ -117,6 +118,8 @@ struct tmio_mmc_host {
>        struct mmc_request      *mrq;
>        struct mmc_data         *data;
>        struct mmc_host         *mmc;
> +       struct clk              *clk;
> +       bool                    clk_is_running;
>        int                     irq;
>
>        /* pio related stuff */
> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> index 6b9c5d0..e405895 100644
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -23,6 +23,7 @@
>  */
>  struct tmio_mmc_data {
>        const unsigned int              hclk;
> +       const char                      *clk_name;

Drop that.

>  };
>
>  /*
> --
> 1.6.2.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Otherwise that patch doesn't seem wrong to me?
As soon as we have the possibility for MFD drivers to provide their
own clk API implementation, this should also work with ASIC3/TMIO.
I'll try to register my ASIC3 HCLK with clkdev and see if this patch
works.

regards
Philipp

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[Magnus Damm]

On Thu, Jul 30, 2009 at 12:20 AM, pHilipp Zabel<philipp.zabel@gmail.com> wrote:
> On Tue, Jul 28, 2009 at 9:51 AM, Guennadi
> Liakhovetski<g.liakhovetski@gmx.de> wrote:
>> If the platform device has a clock associated with the tmio-mmc device,
>> use it.
>>
>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> Cc: Magnus Damm <damm@opensource.se>
>> ---
>>
>> Depends (at least logically) on
>> http://marc.info/?l=linux-kernel&m=124782904228865&w=2
>>
>> Sorry if I forgot anyone who volunteered to help out with MMC patches.
>>
>>  drivers/mmc/host/tmio_mmc.c |   21 +++++++++++++++++++++
>>  drivers/mmc/host/tmio_mmc.h |    3 +++
>>  include/linux/mfd/tmio.h    |    1 +
>>  3 files changed, 25 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
>> index c246191..2a01572 100644
>> --- a/drivers/mmc/host/tmio_mmc.c
>> +++ b/drivers/mmc/host/tmio_mmc.c
>> @@ -567,6 +576,11 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
>>                        goto unmap_cnf;
>>        }
>>
>> +       host->clk = clk_get(&dev->dev, pdata->clk_name);
>
> Instead of pdata->clk_name, this should be either NULL or better
> "HCLK" (to differentiate from CLK32, not sure if we want to be able to
> toggle that, too, in the future).
> Passing the clock consumer ID via pdata is just wrong and shouldn't be
> needed with clkdev.

Really? I think that depends on the clock framework implementation.
Remember that this needs to work on other architectures than just ARM.

Cheers,

/ magnus

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[pHilipp Zabel]

On Wed, Jul 29, 2009 at 5:20 PM, pHilipp Zabel<philipp.zabel@gmail.com> wrote:
> On Tue, Jul 28, 2009 at 9:51 AM, Guennadi
> Liakhovetski<g.liakhovetski@gmx.de> wrote:
>> If the platform device has a clock associated with the tmio-mmc device,
>> use it.
>>
>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> Cc: Magnus Damm <damm@opensource.se>
>> ---
>>
>> Depends (at least logically) on
>> http://marc.info/?l=linux-kernel&m=124782904228865&w=2
>>
>> Sorry if I forgot anyone who volunteered to help out with MMC patches.
>>
>>  drivers/mmc/host/tmio_mmc.c |   21 +++++++++++++++++++++
>>  drivers/mmc/host/tmio_mmc.h |    3 +++
>>  include/linux/mfd/tmio.h    |    1 +
>>  3 files changed, 25 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
>> index c246191..2a01572 100644
>> --- a/drivers/mmc/host/tmio_mmc.c
>> +++ b/drivers/mmc/host/tmio_mmc.c
>> @@ -32,6 +32,7 @@
>>  #include <linux/mmc/host.h>
>>  #include <linux/mfd/core.h>
>>  #include <linux/mfd/tmio.h>
>> +#include <linux/clk.h>
>>
>>  #include "tmio_mmc.h"
>>
>> @@ -57,10 +58,18 @@ static void tmio_mmc_clk_stop(struct tmio_mmc_host *host)
>>        sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~0x0100 &
>>                sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
>>        msleep(10);
>> +       if (!IS_ERR(host->clk) && host->clk_is_running) {
>> +               clk_disable(host->clk);
>> +               host->clk_is_running = false;
>> +       }
>>  }
>>
>>  static void tmio_mmc_clk_start(struct tmio_mmc_host *host)
>>  {
>> +       if (!IS_ERR(host->clk) && !host->clk_is_running &&
>> +           !clk_enable(host->clk))
>> +               host->clk_is_running = true;
>> +
>>        sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, 0x0100 |
>>                sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
>>        msleep(10);
>> @@ -567,6 +576,11 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
>>                        goto unmap_cnf;
>>        }
>>
>> +       host->clk = clk_get(&dev->dev, pdata->clk_name);
>
> Instead of pdata->clk_name, this should be either NULL or better
> "HCLK" (to differentiate from CLK32, not sure if we want to be able to
> toggle that, too, in the future).
> Passing the clock consumer ID via pdata is just wrong and shouldn't be
> needed with clkdev.
>
>> +
>> +       if (!IS_ERR(host->clk) && !clk_enable(host->clk))
>> +               host->clk_is_running = true;
>> +
>>        /* Enable the MMC/SD Control registers */
>>        sd_config_write16(host, CNF_CMD, SDCREN);
>>        sd_config_write32(host, CNF_CTL_BASE,
>> @@ -608,6 +622,11 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
>>        return 0;
>>
>>  unmap_cnf:
>> +       if (!IS_ERR(host->clk)) {
>> +               clk_disable(host->clk);
>> +               host->clk_is_running = false;
>> +               clk_put(host->clk);
>> +       }
>>        if (host->cnf)
>>                iounmap(host->cnf);
>>  unmap_ctl:
>> @@ -632,6 +651,8 @@ static int __devexit tmio_mmc_remove(struct platform_device *dev)
>>                if (host->cnf)
>>                        iounmap(host->cnf);
>>                mmc_free_host(mmc);
>> +               if (!IS_ERR(host->clk))
>> +                       clk_put(host->clk);
>>        }
>>
>>        return 0;
>> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
>> index 45f06aa..a76d237 100644
>> --- a/drivers/mmc/host/tmio_mmc.h
>> +++ b/drivers/mmc/host/tmio_mmc.h
>> @@ -108,6 +108,7 @@
>>                sd_ctrl_write32((host), CTL_STATUS, mask); \
>>        } while (0)
>>
>> +struct clk;
>>
>>  struct tmio_mmc_host {
>>        void __iomem *cnf;
>> @@ -117,6 +118,8 @@ struct tmio_mmc_host {
>>        struct mmc_request      *mrq;
>>        struct mmc_data         *data;
>>        struct mmc_host         *mmc;
>> +       struct clk              *clk;
>> +       bool                    clk_is_running;
>>        int                     irq;
>>
>>        /* pio related stuff */
>> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
>> index 6b9c5d0..e405895 100644
>> --- a/include/linux/mfd/tmio.h
>> +++ b/include/linux/mfd/tmio.h
>> @@ -23,6 +23,7 @@
>>  */
>>  struct tmio_mmc_data {
>>        const unsigned int              hclk;
>> +       const char                      *clk_name;
>
> Drop that.

Actually, this struct can be dropped completely and the HCLK rate can
be determined with clk_get_rate.

>>  };
>>
>>  /*
>> --
>> 1.6.2.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>
> Otherwise that patch doesn't seem wrong to me?
> As soon as we have the possibility for MFD drivers to provide their
> own clk API implementation, this should also work with ASIC3/TMIO.
> I'll try to register my ASIC3 HCLK with clkdev and see if this patch
> works.
>
> regards
> Philipp
>

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[pHilipp Zabel]

On Wed, Jul 29, 2009 at 5:24 PM, Magnus Damm<magnus.damm@gmail.com> wrote:
> On Thu, Jul 30, 2009 at 12:20 AM, pHilipp Zabel<philipp.zabel@gmail.com> wrote:
>> On Tue, Jul 28, 2009 at 9:51 AM, Guennadi
>> Liakhovetski<g.liakhovetski@gmx.de> wrote:
>>> If the platform device has a clock associated with the tmio-mmc device,
>>> use it.
>>>
>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>> Cc: Magnus Damm <damm@opensource.se>
>>> ---
>>>
>>> Depends (at least logically) on
>>> http://marc.info/?l=linux-kernel&m=124782904228865&w=2
>>>
>>> Sorry if I forgot anyone who volunteered to help out with MMC patches.
>>>
>>>  drivers/mmc/host/tmio_mmc.c |   21 +++++++++++++++++++++
>>>  drivers/mmc/host/tmio_mmc.h |    3 +++
>>>  include/linux/mfd/tmio.h    |    1 +
>>>  3 files changed, 25 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
>>> index c246191..2a01572 100644
>>> --- a/drivers/mmc/host/tmio_mmc.c
>>> +++ b/drivers/mmc/host/tmio_mmc.c
>>> @@ -567,6 +576,11 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
>>>                        goto unmap_cnf;
>>>        }
>>>
>>> +       host->clk = clk_get(&dev->dev, pdata->clk_name);
>>
>> Instead of pdata->clk_name, this should be either NULL or better
>> "HCLK" (to differentiate from CLK32, not sure if we want to be able to
>> toggle that, too, in the future).
>> Passing the clock consumer ID via pdata is just wrong and shouldn't be
>> needed with clkdev.
>
> Really? I think that depends on the clock framework implementation.
> Remember that this needs to work on other architectures than just ARM.

Oh, clkdev is an ARM only thing. Maybe it would be worthwile to make
this even more generic instead?

regards
Philipp

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[pHilipp Zabel]

On Tue, Jul 28, 2009 at 9:51 AM, Guennadi
Liakhovetski<g.liakhovetski@gmx.de> wrote:
> If the platform device has a clock associated with the tmio-mmc device,
> use it.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <damm@opensource.se>
> ---
>
> Depends (at least logically) on
> http://marc.info/?l=linux-kernel&m=124782904228865&w=2
>
> Sorry if I forgot anyone who volunteered to help out with MMC patches.
>
>  drivers/mmc/host/tmio_mmc.c |   21 +++++++++++++++++++++
>  drivers/mmc/host/tmio_mmc.h |    3 +++
>  include/linux/mfd/tmio.h    |    1 +
>  3 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
> index c246191..2a01572 100644
> --- a/drivers/mmc/host/tmio_mmc.c
> +++ b/drivers/mmc/host/tmio_mmc.c
> @@ -32,6 +32,7 @@
>  #include <linux/mmc/host.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/tmio.h>
> +#include <linux/clk.h>
>
>  #include "tmio_mmc.h"
>
> @@ -57,10 +58,18 @@ static void tmio_mmc_clk_stop(struct tmio_mmc_host *host)
>        sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~0x0100 &
>                sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
>        msleep(10);
> +       if (!IS_ERR(host->clk) && host->clk_is_running) {
> +               clk_disable(host->clk);
> +               host->clk_is_running = false;
> +       }
>  }
>
>  static void tmio_mmc_clk_start(struct tmio_mmc_host *host)
>  {
> +       if (!IS_ERR(host->clk) && !host->clk_is_running &&
> +           !clk_enable(host->clk))
> +               host->clk_is_running = true;
> +
>        sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, 0x0100 |
>                sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
>        msleep(10);
> @@ -567,6 +576,11 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
>                        goto unmap_cnf;
>        }
>
> +       host->clk = clk_get(&dev->dev, pdata->clk_name);

As long as we are going to ignore the error code, I think the rest of
the patch would be easier on the eyes with
          if (IS_ERR(host->clk))
                  host->clk = NULL;
inserted here.

> +       if (!IS_ERR(host->clk) && !clk_enable(host->clk))
> +               host->clk_is_running = true;
> +
>        /* Enable the MMC/SD Control registers */
>        sd_config_write16(host, CNF_CMD, SDCREN);
>        sd_config_write32(host, CNF_CTL_BASE,
> @@ -608,6 +622,11 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
>        return 0;
>
>  unmap_cnf:
> +       if (!IS_ERR(host->clk)) {
> +               clk_disable(host->clk);
> +               host->clk_is_running = false;
> +               clk_put(host->clk);
> +       }
>        if (host->cnf)
>                iounmap(host->cnf);
>  unmap_ctl:
> @@ -632,6 +651,8 @@ static int __devexit tmio_mmc_remove(struct platform_device *dev)
>                if (host->cnf)
>                        iounmap(host->cnf);
>                mmc_free_host(mmc);
> +               if (!IS_ERR(host->clk))
> +                       clk_put(host->clk);
>        }
>
>        return 0;
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index 45f06aa..a76d237 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -108,6 +108,7 @@
>                sd_ctrl_write32((host), CTL_STATUS, mask); \
>        } while (0)
>
> +struct clk;
>
>  struct tmio_mmc_host {
>        void __iomem *cnf;
> @@ -117,6 +118,8 @@ struct tmio_mmc_host {
>        struct mmc_request      *mrq;
>        struct mmc_data         *data;
>        struct mmc_host         *mmc;
> +       struct clk              *clk;
> +       bool                    clk_is_running;
>        int                     irq;
>
>        /* pio related stuff */
> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> index 6b9c5d0..e405895 100644
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -23,6 +23,7 @@
>  */
>  struct tmio_mmc_data {
>        const unsigned int              hclk;
> +       const char                      *clk_name;
>  };
>
>  /*
> --
> 1.6.2.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Also I quickly tested this on ASIC3 and so far the device hangs if I
remove the code that enables hclk already in mfd_cell->enable (in
asic3.c). I'll try to find out why that happens.

regards
Philipp

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[Ian Molton]

pHilipp Zabel wrote:

>>>  struct tmio_mmc_data {
>>>        const unsigned int              hclk;
>>> +       const char                      *clk_name;
>> Drop that.
> 
> Actually, this struct can be dropped completely and the HCLK rate can
> be determined with clk_get_rate.

Actually it cant because MFD devices have different hclk frequencies, 
and the mmc driver needs to know this. MFD drivers cannot pass the hclk 
rate in using the clk API at present.

-Ian

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[Dmitry Eremin-Solenikov]

Hi!

Sorry for the delay,

2009/7/28 Ian Molton <ian@mnementh.co.uk>:
> Guennadi Liakhovetski wrote:
>
>> Hi Ian
>
> Hi!
>
>> Thanks for the review.
>
> No problem - I hope you dont feel that I am picking on you in particular
> here, this problem is not isolated to your patches.
>
>> I understand your concerns. Of course, the _proper_ solution would be to
>> implement an architecture-independent clock API, something like what
>> clocklib was trying to do. So, yes, if clocklib were in place now, I
>> certainly would have used it.
>
> Which is of course a chicken/egg situation. I dealt with this a number of
> times during the e-series development, and every time, integration with
> mainline has gone more swiftly and with better quality, when the job was
> done properly to begin with.
>
>> I searched for those clocklib submission attempts (Dmitry added to CC).
>
> Also adding RMK to the CC:
>
>> Last one I can find (maybe I missed some) is from July 2008 - more than a
>> year ago. So, looks like our options currently are:
>>
>> 1. wait for new submissions of clocklib - if any are planned
>
> Dmitry: whats the current status of your clocklib work?

It wasn't refreshed since last submission or so, since for ARM we do have
rmk's clocks implementations and I other maintainers (except AVR32 IIRC)
didn't care at all.

>> 3. take over patches from Dmitry and bring them to a state acceptable for
>> mainline
>
> Take over / collaborate with. I'll happily help people push these patches.
>
>> I personally don't have free (as in beer) time to work on 2 or 3. Anyone?
>
> If I can find out what the current state of this stuff is, I'll help get it
> working - I can test / update all the TC6x and T7x MFD devices (and probably
> asic3 too)

I don't think I'll have time to work on/refresh clocklib patches. Feel
free to continue
where I've left them.


-- 
With best wishes
Dmitry

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[Ian Molton]

Dmitry Eremin-Solenikov wrote:

> I don't think I'll have time to work on/refresh clocklib patches. Feel
> free to continue
> where I've left them.

Where is the latest version ?

Re: [PATCH] tmio_mmc: Optionally support using platform clock

[Dmitry Eremin-Solenikov]

On Mon, Aug 03, 2009 at 06:30:26PM +0100, Ian Molton wrote:
> Dmitry Eremin-Solenikov wrote:
> 
> >I don't think I'll have time to work on/refresh clocklib patches. Feel
> >free to continue
> >where I've left them.
> 
> Where is the latest version ?

Hmmm. I currently don't have access to the HDD I used at that moment.
Please use the lastes patches I've sent to LKML/LAKML. I'm sorry that
I'm not very helpful wrt these patches.

-- 
With best wishes
Dmitry