MMC: Make the configuration memory resource optional

MMC: Make the configuration memory resource optional

[Guennadi Liakhovetski]

Add support for tmio_mmc hardware configurations, that lack the cnf io 
area, like SuperH SoCs. With this patch such hardware can pass a single 
ctl io area with the platform data.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
CC: Magnus Damm <damm@opensource.se>
---
Pierre, I know you wanted to step down as a MMC maintainer (thanks for 
your great work btw!), but since we don't have a new one yet, I'm CC-ing 
you.

A version of this patch has previously been submitted by Magnus Damm 
(CCed), but it hasn't been accepted back at 2.6.29 times (about 4 months 
ago). Now this driver extension has become much simpler, so, I think, 
there should be no problem accepting this patch now.

diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
index 91991b4..c246191 100644
--- a/drivers/mmc/host/tmio_mmc.c
+++ b/drivers/mmc/host/tmio_mmc.c
@@ -519,12 +519,12 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	struct mmc_host *mmc;
 	int ret = -EINVAL;
 
-	if (dev->num_resources != 3)
+	if (dev->num_resources < 2 || dev->num_resources > 3)
 		goto out;
 
 	res_ctl = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	res_cnf = platform_get_resource(dev, IORESOURCE_MEM, 1);
-	if (!res_ctl || !res_cnf)
+	if (!res_ctl)
 		goto out;
 
 	pdata = cell->driver_data;
@@ -548,9 +548,11 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	if (!host->ctl)
 		goto host_free;
 
-	host->cnf = ioremap(res_cnf->start, resource_size(res_cnf));
-	if (!host->cnf)
-		goto unmap_ctl;
+	if (res_cnf) {
+		host->cnf = ioremap(res_cnf->start, resource_size(res_cnf));
+		if (!host->cnf)
+			goto unmap_ctl;
+	}
 
 	mmc->ops = &tmio_mmc_ops;
 	mmc->caps = MMC_CAP_4_BIT_DATA;
@@ -606,7 +608,8 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	return 0;
 
 unmap_cnf:
-	iounmap(host->cnf);
+	if (host->cnf)
+		iounmap(host->cnf);
 unmap_ctl:
 	iounmap(host->ctl);
 host_free:
@@ -626,7 +629,8 @@ static int __devexit tmio_mmc_remove(struct platform_device *dev)
 		mmc_remove_host(mmc);
 		free_irq(host->irq, host);
 		iounmap(host->ctl);
-		iounmap(host->cnf);
+		if (host->cnf)
+			iounmap(host->cnf);
 		mmc_free_host(mmc);
 	}
 
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 9fa9985..45f06aa 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -166,18 +166,24 @@ static inline void sd_ctrl_write32(struct tmio_mmc_host *host, int addr,
 static inline void sd_config_write8(struct tmio_mmc_host *host, int addr,
 		u8 val)
 {
+	if (!host->cnf)
+		return;
 	writeb(val, host->cnf + (addr << host->bus_shift));
 }
 
 static inline void sd_config_write16(struct tmio_mmc_host *host, int addr,
 		u16 val)
 {
+	if (!host->cnf)
+		return;
 	writew(val, host->cnf + (addr << host->bus_shift));
 }
 
 static inline void sd_config_write32(struct tmio_mmc_host *host, int addr,
 		u32 val)
 {
+	if (!host->cnf)
+		return;
 	writew(val, host->cnf + (addr << host->bus_shift));
 	writew(val >> 16, host->cnf + ((addr + 2) << host->bus_shift));
 }

Re: MMC: Make the configuration memory resource optional

[Magnus Damm]

On Fri, Jul 17, 2009 at 8:10 PM, Guennadi
Liakhovetski<g.liakhovetski@gmx.de> wrote:
> Add support for tmio_mmc hardware configurations, that lack the cnf io
> area, like SuperH SoCs. With this patch such hardware can pass a single
> ctl io area with the platform data.

Right, this need looks familiar. =)

Have you tested this on any specific platform?

> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> CC: Magnus Damm <damm@opensource.se>
> ---
> Pierre, I know you wanted to step down as a MMC maintainer (thanks for
> your great work btw!), but since we don't have a new one yet, I'm CC-ing
> you.
>
> A version of this patch has previously been submitted by Magnus Damm
> (CCed), but it hasn't been accepted back at 2.6.29 times (about 4 months
> ago). Now this driver extension has become much simpler, so, I think,
> there should be no problem accepting this patch now.

Yes, this patch looks a bit simpler than what I posted earlier. Nice work.

I remember posting a series of patches for the tmio_mmc driver, but
only the fixes were accepted at that point. Is this patch all that is
needed to get tmio_mmc working on your platform, or are you planning
on posting some other patches as well?

Cheers,

/ magnus

Re: [PATCH] MMC: Make the configuration memory resource optional

[Guennadi Liakhovetski]

(forgot the "[PATCH]" marker on submission, re-added now)

On Fri, 17 Jul 2009, Magnus Damm wrote:

> On Fri, Jul 17, 2009 at 8:10 PM, Guennadi
> Liakhovetski<g.liakhovetski@gmx.de> wrote:
> > Add support for tmio_mmc hardware configurations, that lack the cnf io
> > area, like SuperH SoCs. With this patch such hardware can pass a single
> > ctl io area with the platform data.
> 
> Right, this need looks familiar. =)

Your Acked-by would be appreciated:-)

> Have you tested this on any specific platform?

Yep, on migor with the sh7722 CPU.

> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > CC: Magnus Damm <damm@opensource.se>
> > ---
> > Pierre, I know you wanted to step down as a MMC maintainer (thanks for
> > your great work btw!), but since we don't have a new one yet, I'm CC-ing
> > you.
> >
> > A version of this patch has previously been submitted by Magnus Damm
> > (CCed), but it hasn't been accepted back at 2.6.29 times (about 4 months
> > ago). Now this driver extension has become much simpler, so, I think,
> > there should be no problem accepting this patch now.
> 
> Yes, this patch looks a bit simpler than what I posted earlier. Nice work.
> 
> I remember posting a series of patches for the tmio_mmc driver, but
> only the fixes were accepted at that point. Is this patch all that is
> needed to get tmio_mmc working on your platform, or are you planning
> on posting some other patches as well?

Yes, a patch, modifying drivers/mmc/host/Kconfig and migor platform code 
will be submitted later, once this patch is accepted. In fact, maybe it 
would have been better to add SUPERH to tmio_mmc's entry in Kconfig with 
this patch, or, if sh maintainer agrees, we can pull the second patch over 
MMC tree (if / when there is one) too, to make sure the dependency is 
satisfied.

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

Re: [PATCH] MMC: Make the configuration memory resource optional

[Ian Molton]

Guennadi Liakhovetski wrote:
> (forgot the "[PATCH]" marker on submission, re-added now)
> 
> On Fri, 17 Jul 2009, Magnus Damm wrote:
> 
>> On Fri, Jul 17, 2009 at 8:10 PM, Guennadi
>> Liakhovetski<g.liakhovetski@gmx.de> wrote:
>>> Add support for tmio_mmc hardware configurations, that lack the cnf io
>>> area, like SuperH SoCs. With this patch such hardware can pass a single
>>> ctl io area with the platform data.
>> Right, this need looks familiar. =)
> 
> Your Acked-by would be appreciated:-)

Im away tihs weekend - I'll have a look next week when I return.

:-)

Re: [PATCH] MMC: Make the configuration memory resource optional

[Magnus Damm]

[added akpm to the CC]

On Fri, Jul 17, 2009 at 11:34 PM, Guennadi
Liakhovetski<g.liakhovetski@gmx.de> wrote:
> (forgot the "[PATCH]" marker on submission, re-added now)
>
> On Fri, 17 Jul 2009, Magnus Damm wrote:
>
>> On Fri, Jul 17, 2009 at 8:10 PM, Guennadi
>> Liakhovetski<g.liakhovetski@gmx.de> wrote:
>> > Add support for tmio_mmc hardware configurations, that lack the cnf io
>> > area, like SuperH SoCs. With this patch such hardware can pass a single
>> > ctl io area with the platform data.
>>
>> Right, this need looks familiar. =)
>
> Your Acked-by would be appreciated:-)

Since this is a hobby project I prefer to sign off with my swedish address:

Acked-by: Magnus Damm <damm@opensource.se>

>> Have you tested this on any specific platform?
>
> Yep, on migor with the sh7722 CPU.

I've tested it on Migo-R as well and all seems fine.

Can someone please pick this up? akpm? =)

Thanks for your help!

/ magnus

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Guennadi Liakhovetski wrote:
> Add support for tmio_mmc hardware configurations, that lack the cnf io 
> area, like SuperH SoCs. With this patch such hardware can pass a single 
> ctl io area with the platform data.

NAK

I dont like this approach - although it involves minimal changes to the 
driver, it will leave people puzzling over why their accesses to the cnf 
registers do nothing when debugging.

The cnf area is basically clock and power control for devices that have 
it. If I had known of the existence of other tmio devices that didnt do 
it that way when I wrote the driver, it would never have been put in 
there directly.

The *right* way to do this is to use the clk API for clocks and provide 
a small API for power control that the driver will use, if present.

If people want to change the situation in TMIO re: clocks, as I have 
said before, they should start a discussion on how to adapt the clk API. 
  so that more than just the CPU can use it. This will make everyone 
happy all in one go.

I will happily take a patch abstracting power control from tmio-mmc, 
however.

-Ian

Re: MMC: Make the configuration memory resource optional

[Magnus Damm]

Hi Ian,

On Tue, Jul 28, 2009 at 10:55 PM, Ian Molton<ian@mnementh.co.uk> wrote:
> Guennadi Liakhovetski wrote:
>>
>> Add support for tmio_mmc hardware configurations, that lack the cnf io
>> area, like SuperH SoCs. With this patch such hardware can pass a single ctl
>> io area with the platform data.
>
> NAK

That's not a very polite way to begin your email. How about "hey, hi
or good day"?

> I dont like this approach - although it involves minimal changes to the
> driver, it will leave people puzzling over why their accesses to the cnf
> registers do nothing when debugging.

Half a year ago I posted a set of tmio_mmc patches, and the MMC
maintainer kindly picked out the bug fixes and merged them. No problem
there. The feature request to allow the driver to work with a single
memory window (similar to this patch) was NAKed by you in a very
similar way.

One of the reasons for NAKing my patches at that time was that you
didn't have time to review the 100 lines of code. This time you
dislike it because "it will leave people puzzling".

> The cnf area is basically clock and power control for devices that have it.
> If I had known of the existence of other tmio devices that didnt do it that
> way when I wrote the driver, it would never have been put in there directly.
>
> The *right* way to do this is to use the clk API for clocks and provide a
> small API for power control that the driver will use, if present.

Yes, I think everyone wants to use the clock API to control clocks.

However, the clock API does not solve the issue with a single address
window. That's the issue that this patch and my earlier patches are
trying to solve. Which is the issue that you keep on ignoring.

> If people want to change the situation in TMIO re: clocks, as I have said
> before, they should start a discussion on how to adapt the clk API.  so that
> more than just the CPU can use it. This will make everyone happy all in one
> go.

Regardless of how the clock API is implemented, sooner or later the
driver needs to support a single address window if it's going to run
on such hardware. This is not exactly rocket science.

> I will happily take a patch abstracting power control from tmio-mmc,
> however.

I see it as you are blocking progress without any technical reasons.
It's basically exactly the same as half a year ago. And exactly what
has happened with clocklib in that time?

I understand that you want to have clocklib so you can manage clocks
dynamically for your mfd drivers, but in our use case we already have
working clock framework implementations withing our SoC. There is no
need for any dynamic clock registration and unregistration. With that
in mind it can't be very difficult to understand that there is no
point for SoC vendors to work on clocklib. If's mainly an issue for
mfd.

You talk about correctness in the upstream kernel and refuse to
include things because of your special view of correctness. At the
same time you suggest Guennadi and me to keep the patches local
instead of picking up the changes and merging them in your upstream
driver. This local patch suggestion does not help. If we wanted to
have local patches then we wouldn't submit things upstream.

Your role as a maintainer is to work with the community. Just NAKing
and saying you want some highlevel framework merged first is not very
constructive. I recommend you to evaluate your position in the
communtiy. Use git shortlog to compare your own contributions over
time with what Guennadi has done.

/ magnus

Re: MMC: Make the configuration memory resource optional

[Paul Mundt]

On Tue, Jul 28, 2009 at 02:55:07PM +0100, Ian Molton wrote:
> Guennadi Liakhovetski wrote:
> >Add support for tmio_mmc hardware configurations, that lack the cnf io 
> >area, like SuperH SoCs. With this patch such hardware can pass a single 
> >ctl io area with the platform data.
> 
> NAK
> 
> I dont like this approach - although it involves minimal changes to the 
> driver, it will leave people puzzling over why their accesses to the cnf 
> registers do nothing when debugging.
> 
Are you serious? The cnf window does _not exist_ in these cases. How much
more simply do you want it spelled out for you? And there are plenty of
cases where drivers take a variable number of resources for these sorts
of things already in tree. The only one confused here seems to be you.

> The cnf area is basically clock and power control for devices that have 
> it. If I had known of the existence of other tmio devices that didnt do 
> it that way when I wrote the driver, it would never have been put in 
> there directly.
> 
.. devices that have it, yes. It however is entirely an optional area,
and there are those that do not. Your lack of foresight in this area is
entirely unrelated to the issue at hand. These devices exist, you don't
want to deal with them, so we need to figure out how to move on from that
point.

> The *right* way to do this is to use the clk API for clocks and provide 
> a small API for power control that the driver will use, if present.
> 
The right way is to pretend that the area exists when it really doesn't?
As far as tying in the clocks, yes, that can use a virtual name that we
just map out on the CPU side. But as far as the power control, this area
again does not exist, and we will not be doing anything that pretends
otherwise. The driver has to be aware of the fact that this is an
optional area, and deal with that, rather than the other way around.

> If people want to change the situation in TMIO re: clocks, as I have 
> said before, they should start a discussion on how to adapt the clk API. 
>  so that more than just the CPU can use it. This will make everyone 
> happy all in one go.
> 
This is an orthogonal change, and is certainly something that could be
looked at at a later point in time. Presently however this does not
exist, and making that a requirement for something you didn't think of is
quite frankly absurd. The changes as they are are non-invasive, and you
have had ample time to construct technical reasons for why you are
opposed to them.

> I will happily take a patch abstracting power control from tmio-mmc, 
> however.
> 
Which is one of the things that this patch works towards. Quite frankly I
have a hard time understanding what your objections to any of this are.
You didn't consider the fact that these were optional components, and you
reject anything that works in that direction. If you don't want to
support those sorts of devices, that's your problem, and it's no reason
to keep the kernel back. 6 months to review 100 lines, seriously?

We will not now nor at any point maintain a local patchset for devices
that are in the wild for which upstream support mostly exists. The kernel
needs to deal with them, rather than just dealing with the class of
devices it supported when the driver was first merged. Any suggestions
that maintaining local patches is more useful than trying to work with
upstream only reflects on the maintainer's unwillingness to work with
others.

In summary, unless you have some real technical objections, I'll be
merging these patches through my tree in the next merge window. You are
free to NAK away all day long at that point.

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Magnus Damm wrote:
> Hi Ian,
> 
> That's not a very polite way to begin your email. How about "hey, hi
> or good day"?

Hey, hi, good day.

I dont think I wrote anything impolite and NAK is commonly used on 
kernel mailinglists as simply the opposite of Ack. Please stick to the 
technical issues.

> Half a year ago I posted a set of tmio_mmc patches, and the MMC
> maintainer kindly picked out the bug fixes and merged them.

Yes, I remember acking some patches around then.

> The feature request to allow the driver to work with a single
> memory window (similar to this patch) was NAKed by you in a very
> similar way.
> 
> One of the reasons for NAKing my patches at that time was that you
> didn't have time to review the 100 lines of code. This time you
> dislike it because "it will leave people puzzling".

Just to be clear,  I did later review the code, and I didnt like the 
implementation. I'm reasonably sure I said so then, too.

So, on to the present day:

>> The *right* way to do this is to use the clk API for clocks and provide a
>> small API for power control that the driver will use, if present.
> 
> Yes, I think everyone wants to use the clock API to control clocks.

Good. Talk to Dmitry about his clock API patches. Find out why they 
didnt go upstream. I already said I will help with this. I helped get 
the MFD core code merged in the same way. Just stop writing patches that 
avoid doing this properly.

> However, the clock API does not solve the issue with a single address
> window. That's the issue that this patch and my earlier patches are
> trying to solve. Which is the issue that you keep on ignoring.

No, it doesnt. it solves _half_ that problem. The other half would be 
solved by a patch (which I will happily review / modify / apply) that 
removes power switching from the tmio driver.

So now you have a choice of two areas to work on.

> Regardless of how the clock API is implemented, sooner or later the
> driver needs to support a single address window if it's going to run
> on such hardware. This is not exactly rocket science.

Try not to be insulting. And  this is exactly the reason why I wrote:

>> I will happily take a patch abstracting power control from tmio-mmc,
>> however.

Which is your other 'problem' regarging the second address window.

Once both clocks and power switching are implemented properly, the cnf 
window will dissapear completely from the driver. In the case of TCxx 
and t7lx devices, it'll move to the MFD core. For others, it'll move to 
the platform code, but it _will_ just work.

> I see it as you are blocking progress without any technical reasons.

That you choose to ignore my reasons does not invalidate them.

> It's basically exactly the same as half a year ago. And exactly what
> has happened with clocklib in that time?

How have you helped get the clocklib patches merged during that time?

> I understand that you want to have clocklib so you can manage clocks
> dynamically for your mfd drivers, but in our use case we already have
> working clock framework implementations withing our SoC.

Well, unluckily for you, the driver was written prior to my having any 
knowledge of your SoC. At the time, I thought all tmio devices had a cnf 
area. Im not going to rip the code out (thus breaking 3 devices and 6 
platforms) and I'm not going to apply band-aids because I *know* that if 
I do that it'll be the last anyone hears on the matter and clocklib will 
  never be patched to support this case.

> There is no
> need for any dynamic clock registration and unregistration. With that
> in mind it can't be very difficult to understand that there is no
> point for SoC vendors to work on clocklib. If's mainly an issue for
> mfd.

I dont care who *you* think should do the work. If you want to change 
it, then _do_ something.

> You talk about correctness in the upstream kernel and refuse to
> include things because of your special view of correctness.

The code there now is correct for the hardware it handles. You want it 
to handle new hardware in a way that is a hack and will never be fixed up.

> At the
> same time you suggest Guennadi and me to keep the patches local
> instead of picking up the changes and merging them in your upstream
> driver. This local patch suggestion does not help. If we wanted to
> have local patches then we wouldn't submit things upstream.

I've wanted to submit things upstream before that upstream felt should 
be done another way. Oddly enough, I ended up rewriting them 9 times out 
of 10. Your patches are small, and wont go stale quickly. Which leaves 
you with:

* working hardware.
* time to do it properly.

> Your role as a maintainer is to work with the community. Just NAKing
> and saying you want some highlevel framework merged first is not very
> constructive.

Clocklib is already merged. It needs modifying.

> I recommend you to evaluate your position in the
> communtiy. Use git shortlog to compare your own contributions over
> time with what Guennadi has done.

Im not going to enter a pissing contest over this.

I've made clear the path I'd like people to take if they want to remove 
the second io area from the driver 6 months ago. I've now  seen about 4 
different approaches to doing it as a hack. If the same effort had been 
put into doing it properly, we wouldnt be having this 'discussion'.

-Ian

Re: MMC: Make the configuration memory resource optional

[Mark Brown]

On Wed, Jul 29, 2009 at 11:48:51AM +0900, Magnus Damm wrote:

> I understand that you want to have clocklib so you can manage clocks
> dynamically for your mfd drivers, but in our use case we already have
> working clock framework implementations withing our SoC. There is no
> need for any dynamic clock registration and unregistration. With that
> in mind it can't be very difficult to understand that there is no
> point for SoC vendors to work on clocklib. If's mainly an issue for
> mfd.

While it's true that this doesn't bother SoCs the fact that most clock
API implementations don't allow any off-chip drivers to register clocks
renders the clock API essentially unusable for fairly large parts of the
kernel.  This is especially problematic where the clocks cross from
the SoC domain to the off-SoC domain and clocks from each domain may be
used interchangably.

Looking at the original patch I'm not sure exactly why it runs into
clock API issues so I'm not sure if this is a relevant concern or not
here but I'm mentioning it since I'd kind of expect an impact on the
SoCs from addressing it due to the way the clock API functions are
currently provided.

Re: MMC: Make the configuration memory resource optional

[Magnus Damm]

On Wed, Jul 29, 2009 at 8:58 PM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Jul 29, 2009 at 11:48:51AM +0900, Magnus Damm wrote:
>
>> I understand that you want to have clocklib so you can manage clocks
>> dynamically for your mfd drivers, but in our use case we already have
>> working clock framework implementations withing our SoC. There is no
>> need for any dynamic clock registration and unregistration. With that
>> in mind it can't be very difficult to understand that there is no
>> point for SoC vendors to work on clocklib. If's mainly an issue for
>> mfd.
>
> While it's true that this doesn't bother SoCs the fact that most clock
> API implementations don't allow any off-chip drivers to register clocks
> renders the clock API essentially unusable for fairly large parts of the
> kernel.  This is especially problematic where the clocks cross from
> the SoC domain to the off-SoC domain and clocks from each domain may be
> used interchangably.

Yeah, clocks outside the SoC are not well supported today. From what
I've seen, most embedded boards come with external chips for cameras,
audio codecs and/or phy devices. These devices often get their clocks
from the main SoC. Allowing the drivers for such chips to use the
clock framework to register clocks for internal divisors would allow
driver writers to write better code which in turn would make life
easier for most people hacking on embedded kernels.

So I'm all for working towards allowing off-chip drivers to register
and unregister clocks.

The problem with the clock framework API is that the data structures
varies depending on implementation. So the ops callback structure on
SuperH is different compared to ARM. I suspect that adding generic
clocklib support across the architectures will take quite a bit of
time to implement propely.

> Looking at the original patch I'm not sure exactly why it runs into
> clock API issues so I'm not sure if this is a relevant concern or not
> here but I'm mentioning it since I'd kind of expect an impact on the
> SoCs from addressing it due to the way the clock API functions are
> currently provided.

In my opinion this patch has nothing to do with the clock framework.

But fixing up clocklib properly would certainly be beneficial for
everyone. Holding the driver hostage until clocklib is upstream
however, that's just silly.

Cheers,

/ magnus

Re: MMC: Make the configuration memory resource optional

[Paul Mundt]

On Wed, Jul 29, 2009 at 09:27:54PM +0900, Magnus Damm wrote:
> On Wed, Jul 29, 2009 at 8:58 PM, Mark
> Brown<broonie@opensource.wolfsonmicro.com> wrote:
> > Looking at the original patch I'm not sure exactly why it runs into
> > clock API issues so I'm not sure if this is a relevant concern or not
> > here but I'm mentioning it since I'd kind of expect an impact on the
> > SoCs from addressing it due to the way the clock API functions are
> > currently provided.
> 
> In my opinion this patch has nothing to do with the clock framework.
> 
> But fixing up clocklib properly would certainly be beneficial for
> everyone. Holding the driver hostage until clocklib is upstream
> however, that's just silly.
> 
It also presupposes that people want clocklib upstream. The last time I
saw it pass through my inbox, I wasn't convinced that it really bought us
anything. The ARM clkdev thing on the other hand is something I plan to
drag in on the SH side as well, but that too is a separate thing.

If folks are of the mindset that the current patch is a misuse of the
clock framework, then the objection needs to be specifically noted. I'm
willing to make concessions on the clock framework side if Ian has
problems with the current scheme, but I am not at all convinced of the
relative merit of clocklib, or holding drivers hostage to such. 

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Mark Brown wrote:

Hi Mark,

> Looking at the original patch I'm not sure exactly why it runs into
> clock API issues so I'm not sure if this is a relevant concern or not
> here

The problem for tmio-mmc is that its an MFD driver. The idea behind the 
MFD framework was to allow drivers that work on similar hardware to work 
both independantly and as part of a MFD, which often have extra core logic.

Using the clock API is problematic, then, because for MFD based users of 
tmio-mmc, the clock API isnt useable, but for non-MFD users, it is.

tmio-mmc has (currently) some code in it that uses a second IO range to 
control both clocks and power on the toshiba family of MFDs. This 
ideally would be replaced by the clock API and a bunch of power control 
callbacks that would abstract it out completely from the tmio-mmc driver 
and into either platform or MFD core code depending on what the user of 
the driver is.

IOW, using the clock API will fix it for everyone.

 > bI'd kind of expect an impact on the
> SoCs from addressing it due to the way the clock API functions are
> currently provided.

Quite.

-Ian

Re: MMC: Make the configuration memory resource optional

[Mark Brown]

On Wed, Jul 29, 2009 at 09:27:54PM +0900, Magnus Damm wrote:
> On Wed, Jul 29, 2009 at 8:58 PM, Mark
> Brown<broonie@opensource.wolfsonmicro.com> wrote:

> > While it's true that this doesn't bother SoCs the fact that most clock
> > API implementations don't allow any off-chip drivers to register clocks
> > renders the clock API essentially unusable for fairly large parts of the

> Yeah, clocks outside the SoC are not well supported today. From what
> I've seen, most embedded boards come with external chips for cameras,
> audio codecs and/or phy devices. These devices often get their clocks
> from the main SoC. Allowing the drivers for such chips to use the
> clock framework to register clocks for internal divisors would allow
> driver writers to write better code which in turn would make life
> easier for most people hacking on embedded kernels.

That's not actually abundantly clear for the audio stuff, or rather the
audio stuff would like additional features like constraint based
configuration.

> The problem with the clock framework API is that the data structures
> varies depending on implementation. So the ops callback structure on
> SuperH is different compared to ARM. I suspect that adding generic
> clocklib support across the architectures will take quite a bit of
> time to implement propely.

Indeed.  It's actually much worse than you say, each individual ARM
architecture has its own clock API implementation of varying quality and
of course there are architectures that don't do the clock API at all.

Re: MMC: Make the configuration memory resource optional

[Magnus Damm]

On Wed, Jul 29, 2009 at 9:42 PM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Jul 29, 2009 at 09:27:54PM +0900, Magnus Damm wrote:
>> On Wed, Jul 29, 2009 at 8:58 PM, Mark
>> Brown<broonie@opensource.wolfsonmicro.com> wrote:
>
>> > While it's true that this doesn't bother SoCs the fact that most clock
>> > API implementations don't allow any off-chip drivers to register clocks
>> > renders the clock API essentially unusable for fairly large parts of the
>
>> Yeah, clocks outside the SoC are not well supported today. From what
>> I've seen, most embedded boards come with external chips for cameras,
>> audio codecs and/or phy devices. These devices often get their clocks
>> from the main SoC. Allowing the drivers for such chips to use the
>> clock framework to register clocks for internal divisors would allow
>> driver writers to write better code which in turn would make life
>> easier for most people hacking on embedded kernels.
>
> That's not actually abundantly clear for the audio stuff, or rather the
> audio stuff would like additional features like constraint based
> configuration.

Without knowing too much about this, wouldn't camera sensors want
similar features?

>> The problem with the clock framework API is that the data structures
>> varies depending on implementation. So the ops callback structure on
>> SuperH is different compared to ARM. I suspect that adding generic
>> clocklib support across the architectures will take quite a bit of
>> time to implement propely.
>
> Indeed.  It's actually much worse than you say, each individual ARM
> architecture has its own clock API implementation of varying quality and
> of course there are architectures that don't do the clock API at all.

Yeah. This is exactly why I don't want to block on the clocklib implementation.

Cheers,

/ magnus

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Magnus Damm wrote:

>> Indeed.  It's actually much worse than you say, each individual ARM
>> architecture has its own clock API implementation of varying quality and
>> of course there are architectures that don't do the clock API at all.
> 
> Yeah. This is exactly why I don't want to block on the clocklib implementation.

Yeah, good idea... lets ignore the problem until its so big we cant fix 
it at all...

Re: MMC: Make the configuration memory resource optional

[Mark Brown]

On Wed, Jul 29, 2009 at 09:51:30PM +0900, Magnus Damm wrote:
> On Wed, Jul 29, 2009 at 9:42 PM, Mark
> Brown<broonie@opensource.wolfsonmicro.com> wrote:

> > That's not actually abundantly clear for the audio stuff, or rather the
> > audio stuff would like additional features like constraint based
> > configuration.

> Without knowing too much about this, wouldn't camera sensors want
> similar features?

They may well but I'm not familiar with them; I can speak much more
confidently about audio.

Re: MMC: Make the configuration memory resource optional

[Magnus Damm]

On Wed, Jul 29, 2009 at 9:58 PM, Ian Molton<ian@mnementh.co.uk> wrote:
> Magnus Damm wrote:
>
>>> Indeed.  It's actually much worse than you say, each individual ARM
>>> architecture has its own clock API implementation of varying quality and
>>> of course there are architectures that don't do the clock API at all.
>>
>> Yeah. This is exactly why I don't want to block on the clocklib
>> implementation.
>
> Yeah, good idea... lets ignore the problem until its so big we cant fix it
> at all...

Note that I don't need clocklib to get the tmio_mmc driver working for
my platform. It's something _you_ need for the MFD chips. But you seem
to want me to fix it for you even though I don't have any particular
need for it.

/ magnus

Re: MMC: Make the configuration memory resource optional

[Mark Brown]

On 29 Jul 2009, at 13:58, Ian Molton <ian@mnementh.co.uk> wrote:

> Magnus Damm wrote:
>
>>> Indeed.  It's actually much worse than you say, each individual ARM
>>> architecture has its own clock API implementation of varying  
>>> quality and
>>> of course there are architectures that don't do the clock API at  
>>> all.
>> Yeah. This is exactly why I don't want to block on the clocklib  
>> implementation.
>
> Yeah, good idea... lets ignore the problem until its so big we cant  
> fix it at all...

The problem here is sufficiently substantial that I'd be impressed if  
we managed to get a good solution in within a single kernel release.  
This does help encourage people to keep local hacks but those are much  
more realistic.

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Magnus Damm wrote:

> Note that I don't need clocklib to get the tmio_mmc driver working for
> my platform. It's something _you_ need for the MFD chips. But you seem
> to want me to fix it for you even though I don't have any particular
> need for it.


Actually, the tmio-mmc driver works perfectly on the MFD chips right 
now. These are the chips it was written to handle.

YOU want to change it, not me. Don't try to turn the argument around.

One more of these "its all your fault and you're deliberately blocking 
me" whines and I *will* start doing just that.

Can we keep it technical now, please?

-Ian (pissed off now)

Re: MMC: Make the configuration memory resource optional

[Paul Mundt]

On Wed, Jul 29, 2009 at 02:51:36PM +0100, Ian Molton wrote:
> Magnus Damm wrote:
> >Note that I don't need clocklib to get the tmio_mmc driver working for
> >my platform. It's something _you_ need for the MFD chips. But you seem
> >to want me to fix it for you even though I don't have any particular
> >need for it.
> 
> Actually, the tmio-mmc driver works perfectly on the MFD chips right 
> now. These are the chips it was written to handle.
> 
And these patches are fixing up the mmc driver to support the non-MFD
case. If you want to fix up the MFD driver to expose a similar interface
to the mmc one as what we are doing with the clock framework, that is
fine, but is likewise an unrelated change.

Lets evaluate the clock options we have today:

	1) clock framework
	2) clkdev
	3) clocklib

#1 is what these patches are written for, and the only standard in-tree
interface that we have cross platform today. #2 could be generalized, but
that needs discussion by itself, as it was never proposed as a standard
interface and never submitted to l-k for review. #3 is not in the kernel
today and it's not clear that it ever will be.

> YOU want to change it, not me. Don't try to turn the argument around.
> 
We wish to make constructive changes to the MMC driver to accomodate
devices you hadn't considered. It is not an MFD in our case, we have no
ability to test the MFD code, and we will not be making any changes to
the MFD code. You are the one with the MFD, you get to handle it. While
we will work with you to make sure nothing on the MFD side breaks,
holding the MMC driver hostage until MFD is reworked or some random bits
of unrelated infrastructure are merged is not constructive.

If you can show what is wrong with how the clock framework is being used
in the patch that Guennadi posted, we will certainly rework it as
necessary. However, I will not at this point be merging clkdev in to my
architecture tree, and clocklib I have never supported. These are things
that can be done and supported incrementally, but making these
prerequisites is simply blocking progress, especially when there is no
consensus on the clkdev/clocklib parts.

> Can we keep it technical now, please?
> 
So far you have not produced a technical rebuttal to any of the patches.

You object to #1 because you think it confuses things, despite the fact
that in our case this cnf window just doesn't exist at all, and there are
plenty of existing cases in the kernel today where variable number of
resources are handled for fairly similar situations. We have no intention
of pretending the resource exists when it does not. However you want to
rework the MFD driver to support clocks and power control is entirely up
to you, but none of that has any real bearing on the MMC driver.

Your objections to #2 are non-obvious, since you haven't stated any other
than the fact you would like to see it solved using APIs that do not
exist in the kernel. Perhaps in the long term we can move towards clkdev
or clocklib if they are proposed as generic interfaces and merged in to
the kernel at some point in the future, but today the clock framework is
what we have to work with, and that is what we will be working with.

If you don't want to expand on either one of those points, then I can
just include your NAKed-by in the commit logs and we can take it from
there. You can always maintain a local patchset that drops support for
non-MFD chips.

Re: MMC: Make the configuration memory resource optional

[pHilipp Zabel]

On Wed, Jul 29, 2009 at 10:17 PM, Paul Mundt<lethal@linux-sh.org> wrote:
> On Wed, Jul 29, 2009 at 02:51:36PM +0100, Ian Molton wrote:
>> Magnus Damm wrote:
>> >Note that I don't need clocklib to get the tmio_mmc driver working for
>> >my platform. It's something _you_ need for the MFD chips. But you seem
>> >to want me to fix it for you even though I don't have any particular
>> >need for it.
>>
>> Actually, the tmio-mmc driver works perfectly on the MFD chips right
>> now. These are the chips it was written to handle.
>>
> And these patches are fixing up the mmc driver to support the non-MFD
> case. If you want to fix up the MFD driver to expose a similar interface
> to the mmc one as what we are doing with the clock framework, that is
> fine, but is likewise an unrelated change.
>
> Lets evaluate the clock options we have today:
>
>        1) clock framework
>        2) clkdev

2) is nothing more than an implementation detail of 1). How clk_get
looks up the struct clk internally should not be of any concern to the
consumer.

>        3) clocklib
>
> #1 is what these patches are written for, and the only standard in-tree
> interface that we have cross platform today. #2 could be generalized, but
> that needs discussion by itself, as it was never proposed as a standard
> interface and never submitted to l-k for review. #3 is not in the kernel
> today and it's not clear that it ever will be.

Yes, 3) is unlikely to happen in the near future.

[...]
> If you can show what is wrong with how the clock framework is being used
> in the patch that Guennadi posted, we will certainly rework it as
> necessary. However, I will not at this point be merging clkdev in to my
> architecture tree, and clocklib I have never supported. These are things
> that can be done and supported incrementally, but making these
> prerequisites is simply blocking progress, especially when there is no
> consensus on the clkdev/clocklib parts.

Providing the clock consumer ID via platform data is wrong. As I
understand it, that ID should be either NULL if the clock can be
determined from the struct device pointer (i.e. if it's the only clock
provided to that device), or it should be used to distinguish the
device's clock input pins (in the tmio-mmc case that would be 'hclk'
or 'clk32' if I remember the datasheet correctly).

>> Can we keep it technical now, please?

[...]

regards
Philipp

Re: MMC: Make the configuration memory resource optional

[Paul Mundt]

On Wed, Jul 29, 2009 at 10:55:31PM +0200, pHilipp Zabel wrote:
> On Wed, Jul 29, 2009 at 10:17 PM, Paul Mundt<lethal@linux-sh.org> wrote:
> > On Wed, Jul 29, 2009 at 02:51:36PM +0100, Ian Molton wrote:
> >> Magnus Damm wrote:
> >> >Note that I don't need clocklib to get the tmio_mmc driver working for
> >> >my platform. It's something _you_ need for the MFD chips. But you seem
> >> >to want me to fix it for you even though I don't have any particular
> >> >need for it.
> >>
> >> Actually, the tmio-mmc driver works perfectly on the MFD chips right
> >> now. These are the chips it was written to handle.
> >>
> > And these patches are fixing up the mmc driver to support the non-MFD
> > case. If you want to fix up the MFD driver to expose a similar interface
> > to the mmc one as what we are doing with the clock framework, that is
> > fine, but is likewise an unrelated change.
> >
> > Lets evaluate the clock options we have today:
> >
> > ? ? ? ?1) clock framework
> > ? ? ? ?2) clkdev
> 
> 2) is nothing more than an implementation detail of 1). How clk_get
> looks up the struct clk internally should not be of any concern to the
> consumer.
> 
For the sake of the driver, sure. On the architecture side it's a bit
more work. It is on my TODO list to add to the SH clock framework, but it
will take some work, and is out of scope for 2.6.32.

> [...]
> > If you can show what is wrong with how the clock framework is being used
> > in the patch that Guennadi posted, we will certainly rework it as
> > necessary. However, I will not at this point be merging clkdev in to my
> > architecture tree, and clocklib I have never supported. These are things
> > that can be done and supported incrementally, but making these
> > prerequisites is simply blocking progress, especially when there is no
> > consensus on the clkdev/clocklib parts.
> 
> Providing the clock consumer ID via platform data is wrong. As I
> understand it, that ID should be either NULL if the clock can be
> determined from the struct device pointer (i.e. if it's the only clock
> provided to that device), or it should be used to distinguish the
> device's clock input pins (in the tmio-mmc case that would be 'hclk'
> or 'clk32' if I remember the datasheet correctly).
> 
If that's the only issue, then yes, no problem. I agree that passing in
the clock string is not something we really want to be doing, so using
the virtualized clock name here sounds fine. We can mangle it on the
platform side until the clkdev implementation is completed, but none of
that matters to the driver at that point.

This should be taken care of in the next iteration of the patch. Thanks
for your comments!

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Ok I've spent some time thinking on this.

There is _no_ clean solution at present and Im not happy with having 
more than one clocking system co-existing in the mmc driver.

I will produce a patch to  completely remove all clocking and power 
control from the driver and make it use callbacks in order to achieve 
this. This will leave us with a clean driver that will be simple to 
adapt to the clock API.

I wont have time to work on it until next week, however, so if anyone 
wants to take a stab at it in the meantime, feel free to do so, and I'll 
review it next week.

-Ian

Re: MMC: Make the configuration memory resource optional

[Guennadi Liakhovetski]

On Thu, 30 Jul 2009, Ian Molton wrote:

> Ok I've spent some time thinking on this.
> 
> There is _no_ clean solution at present and Im not happy with having more than
> one clocking system co-existing in the mmc driver.
> 
> I will produce a patch to  completely remove all clocking and power control
> from the driver and make it use callbacks in order to achieve this. This will
> leave us with a clean driver that will be simple to adapt to the clock API.
> 
> I wont have time to work on it until next week, however, so if anyone wants to
> take a stab at it in the meantime, feel free to do so, and I'll review it next
> week.

While you're at it, please, consider swapping the two lines in 
tmio_mmc_probe():

-	tmio_mmc_clk_stop(host);
	reset(host);
+	tmio_mmc_clk_stop(host);

Otherwise, I think, reset causes problems trying to access the controller 
with disabled clock. At least this is needed on SuperH.

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

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Guennadi Liakhovetski wrote:

> While you're at it, please, consider swapping the two lines in 
> tmio_mmc_probe():
> 
> -	tmio_mmc_clk_stop(host);
> 	reset(host);
> +	tmio_mmc_clk_stop(host);
> 
> Otherwise, I think, reset causes problems trying to access the controller 
> with disabled clock. At least this is needed on SuperH.

Interesting. I'll see what the result of this is on TMIO - This sequence 
was garnered from the WinCE driver for the chip.

I cant see _why_ this should be a problem, as this disables the card 
clock, not HCLK. Could you debug further in tmio_mmc_clk_stop() please 
and see if reordering only one of the two IO accesses cures this?

Let me know what you find out.

-Ian

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Ok, I took another look just now (aa few minutes break)

One slight thorn is that the TMIO MFDs have a 1:1 card clock mode where 
the divisor is disabled. Annoyingly this is set up by a bit in the CNF 
IO space

How do non-TMIO (eg. superH) tmio cells select this mode? or do they 
simply not have a 1:1 ratio available to them?

(Im trying to decide whether to have a 'set 1:1' callback, or a 'set 
clock' callback. The latter is thorny because it'd involve the MFD core 
also mapping the CTL area.

-Ian

Re: MMC: Make the configuration memory resource optional

[Guennadi Liakhovetski]

On Thu, 30 Jul 2009, Ian Molton wrote:

> Guennadi Liakhovetski wrote:
> 
> > While you're at it, please, consider swapping the two lines in
> > tmio_mmc_probe():
> > 
> > -	tmio_mmc_clk_stop(host);
> > 	reset(host);
> > +	tmio_mmc_clk_stop(host);
> > 
> > Otherwise, I think, reset causes problems trying to access the controller
> > with disabled clock. At least this is needed on SuperH.
> 
> Interesting. I'll see what the result of this is on TMIO - This sequence was
> garnered from the WinCE driver for the chip.
> 
> I cant see _why_ this should be a problem, as this disables the card clock,
> not HCLK. Could you debug further in tmio_mmc_clk_stop() please and see if
> reordering only one of the two IO accesses cures this?

Not sure I understood the "reordering only one of the two IO accesses" 
correctly, but I swapped the two sd_ctrl_write16() calls in 
tmio_mmc_clk_stop() and no, it didn't cure the problem.

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

Re: MMC: Make the configuration memory resource optional

[Magnus Damm]

On Fri, Jul 31, 2009 at 4:21 AM, Ian Molton<ian@mnementh.co.uk> wrote:
> Guennadi Liakhovetski wrote:
>
>> While you're at it, please, consider swapping the two lines in
>> tmio_mmc_probe():
>>
>> -       tmio_mmc_clk_stop(host);
>>        reset(host);
>> +       tmio_mmc_clk_stop(host);
>>
>> Otherwise, I think, reset causes problems trying to access the controller
>> with disabled clock. At least this is needed on SuperH.
>
> Interesting. I'll see what the result of this is on TMIO - This sequence was
> garnered from the WinCE driver for the chip.
>
> I cant see _why_ this should be a problem, as this disables the card clock,
> not HCLK. Could you debug further in tmio_mmc_clk_stop() please and see if
> reordering only one of the two IO accesses cures this?

I wonder if the clock framework patch from Guennadi ties in the clock
that drives the TMIO block, instead of the interface clock that is
used to communicate with the physical media? That would explain the
reordering of the tmio_mmc_clk_stop() function.

In my mind, using the clock framework to control the interface clock
sounds like a good plan. As for the clock that drives the TMIO block
itself (that Guennadi's patch tries to control) - on a second thought
it may make more sense to use the upcoming Runtime PM framework to
control that one.

Making use of the Runtime PM comes with a big advantage - it will
provide use with hooks to allow saving and restoring register context
so the power domain containing the TMIO block can be powered off
during runtime.

I'd be happy to fix up the Runtime PM related parts of the tmio_mmc
driver if you'd like. The Runtime PM changes will of course "just
work" in the MFD case - ie do nothing unless your architecture has
support for it.

The recently posted Runtime PM patch for the SuperH Mobile I2C driver
may serve as an example:
http://patchwork.kernel.org/patch/38514/

Another solution is of course to use two clocks in TMIO driver - one
for the interface clock and one for the hardware block itself. This
may be better if you want to control the hardware block clocks from
the MFD layer in the future. Let me know what you think.

Thanks for your help.

Cheers,

/ magnus

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Guennadi Liakhovetski wrote:

>> I cant see _why_ this should be a problem, as this disables the card clock,
>> not HCLK. Could you debug further in tmio_mmc_clk_stop() please and see if
>> reordering only one of the two IO accesses cures this?
> 
> Not sure I understood the "reordering only one of the two IO accesses" 
> correctly, but I swapped the two sd_ctrl_write16() calls in 
> tmio_mmc_clk_stop() and no, it didn't cure the problem.

I meant can you reorder them so that only one or the other is after the 
reset. Thus eliminating one  (perhaps) as the cause of the problem.

Does your chip actually use the tmio-type reset, or has it a hard reset 
line or something?

Also is your issue that the driver doesnt work, or that you cant access 
registers from something like userspace ?

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

I still do not have info from people about wether non MFD users of the 
tmio-mmc driver have a 1:1 host/card clock capability.

I could really use the information if anyone expects me to sort this 
situation out.

Re: MMC: Make the configuration memory resource optional

[Magnus Damm]

Hi Ian,

On Wed, Aug 5, 2009 at 3:21 AM, Ian Molton<ian@mnementh.co.uk> wrote:
> I still do not have info from people about wether non MFD users of the
> tmio-mmc driver have a 1:1 host/card clock capability.
>
> I could really use the information if anyone expects me to sort this
> situation out.

Sorry for not answering your question earlier. This may sound starnge,
but I don't have any datasheet. I do however think that you can assume
a 1:1 host/card clock capability, at least to begin with.

So please use the clock framework to enable/disable the clock and get
the rate, and we'll do our best to adjust the architecture code after
that. And I'll cook up a patch for Runtime PM on top of that if that's
ok with you.

Thanks.

/ magnus

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Magnus Damm wrote:

> Sorry for not answering your question earlier. This may sound starnge,
> but I don't have any datasheet. I do however think that you can assume
> a 1:1 host/card clock capability, at least to begin with.

Im not so sure I can - the 1:1 clock on MFD is selected via the CNF IO 
area and as such the other users of the driver that dont have CNF areas 
cannot possibly be using this method. Do you have your own method for 
clock selection, or are you using the routine in tmio-mmc.c - if the 
latter then you are certainly not running a 1:1 clock

> So please use the clock framework to enable/disable the clock and get
> the rate, and we'll do our best to adjust the architecture code after
> that. And I'll cook up a patch for Runtime PM on top of that if that's
> ok with you.

I _cant_ use the clock framework by itself in tmio-mmc because that 
would break each and every MFD user of the driver.

-Ian

Re: MMC: Make the configuration memory resource optional

[Guennadi Liakhovetski]

Hi Ian

On Mon, 3 Aug 2009, Ian Molton wrote:

> Guennadi Liakhovetski wrote:
> 
> > > I cant see _why_ this should be a problem, as this disables the card
> > > clock,
> > > not HCLK. Could you debug further in tmio_mmc_clk_stop() please and see if
> > > reordering only one of the two IO accesses cures this?
> > 
> > Not sure I understood the "reordering only one of the two IO accesses"
> > correctly, but I swapped the two sd_ctrl_write16() calls in
> > tmio_mmc_clk_stop() and no, it didn't cure the problem.
> 
> I meant can you reorder them so that only one or the other is after the reset.
> Thus eliminating one  (perhaps) as the cause of the problem.

With my patches the tmio_mmc_clk_stop() function looked like this 
(pseudocode):

tmio_mmc_clk_stop()
{
	CTL_CLK_AND_WAIT_CTL = 0x0000;
	msleep(10);
	CTL_SD_CARD_CLK_CTL &= ~0x0100;
	msleep(10);
	clk_disable(clk);
}

I splitted the clk_disable() call out in a separate function and moved 
_only_ it after the reset() call - it worked too. Does this answer your 
question?

> Does your chip actually use the tmio-type reset, or has it a hard reset line
> or something?

No idea. As you know, this is not a separate chip, it is a built in 
controller into the sh7722 (probably, also other) SuperH SoC, so, I doubt 
it has a separate external reset line. Just like Magnus I have no 
datasheet for that block in SoC.

> Also is your issue that the driver doesnt work, or that you cant access
> registers from something like userspace ?

The driver doesn't work. Which means in this case - no interrupts, the mmc 
tasklet in in "D."

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Hi all,

Could all non-MFD users of tmio-mmc please ensure that the following is 
actually true on your hardware?

   mmc->f_max = pdata->hclk;
   mmc->f_min = mmc->f_max / 512;

According to all the data I have, the smallest divider setting of 0x100 
is /2 and only MFD controllers with the CNF area can select no-divider mode.

this would mean that non-MFD chips should be doing:

   mmc->f_max = pdata->hclk / 2;
   mmc->f_min = pdata->hclk / 512;

Example idea for how to solve the clock/cnf problem.

[Ian Molton]

Hi folks,

I've spent a little time hacking on the code this afternoon, here is a 
WIP that completely removes cnf area accesses from the mmc driver. These 
are now pushed out into the platform / MFD level code. I've taken 
TC6393XB as an example and pushed the CNF code into that. I imagine that 
several chips can share that code so I will break it out into another 
file, mfd/tmio_common.c, probably.

This code probably doesnt compile yet, but should give some idea about 
how I think this should be done.

How are other devices setting the card clock? are they using my clock 
setup function? If so, then we are probably best off avoiding the clock 
API for the card clock, as getting the full range of frequencies require 
access to both CNF and CTL areas on TMIO MFDs.

If not, then we will need to do more thinking.

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Guennadi Liakhovetski wrote:
> Hi Ian

Hi,

> With my patches the tmio_mmc_clk_stop() function looked like this 
> (pseudocode):
> 
> tmio_mmc_clk_stop()
> {
> 	CTL_CLK_AND_WAIT_CTL = 0x0000;
> 	msleep(10);
> 	CTL_SD_CARD_CLK_CTL &= ~0x0100;
> 	msleep(10);
> 	clk_disable(clk);
> }
> 
> I splitted the clk_disable() call out in a separate function and moved 
> _only_ it after the reset() call - it worked too. Does this answer your 
> question?

What is clk? HCLK?

If so, then I'm not surprised it didn't work. tmio_mmc_clk_stop/start() 
are for controlling the card clock.

What you have done is too disable the host clock when you disable the 
card clock. Frankly, I'm amazed it worked at all.

The correct fix would be to remove the clk_disable(clk) from your 
function. The host clock should be running constantly (unless 
suspended). The card clock is gated.

You should probably also check the placement of wherever your 
clk_enable() is too.

-Ian

Re: MMC: Make the configuration memory resource optional

[Guennadi Liakhovetski]

On Wed, 5 Aug 2009, Ian Molton wrote:

> Hi all,
> 
> Could all non-MFD users of tmio-mmc please ensure that the following is
> actually true on your hardware?
> 
>   mmc->f_max = pdata->hclk;
>   mmc->f_min = mmc->f_max / 512;

That's what I currently have in the driver, yes.

> According to all the data I have, the smallest divider setting of 0x100 is /2
> and only MFD controllers with the CNF area can select no-divider mode.
> 
> this would mean that non-MFD chips should be doing:
> 
>   mmc->f_max = pdata->hclk / 2;
>   mmc->f_min = pdata->hclk / 512;

How do we verify this? Do I need some "very fast" card, so that sd would 
try to drive the clock beyond pdata->hclk / 2, which should then fail? 
Currently I'm using 24MHz (copied from the original driver), and the only 
thing I know about the controller is its "Maximum operating frequency: 25 
MHz."

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Guennadi Liakhovetski wrote:

> How do we verify this? Do I need some "very fast" card, so that sd would 
> try to drive the clock beyond pdata->hclk / 2, which should then fail? 

One way would be a 'scope on the card pins, but since you havent said I 
am assuming you have not altered the clock-speed setting routine.

Here you can see why I didnt like the silently-drop-conf-accesses 
version of the patch.  If you had not made conf area accesses silently 
succeed, you'd have found one lurking in the set_clock function.

Since your controller has no CNF area, it was discarding the write to 
the 1:1 clock bit (that isnt  in its none-existant conf area) and thus 
you got the next lowest clock as a result (/2)

> Currently I'm using 24MHz (copied from the original driver), and the only 
> thing I know about the controller is its "Maximum operating frequency: 25 
> MHz."

Thats the HCLK frequency, not the card clock.

the TMIO MFD devices can divide this HCLK by anything from 512 to 2, and 
  MFD devices have a facility to disable the divider, yielding full HCLK 
speed as the card clock.

If you havent already found a 1:1 clock enable bit on your device, you 
might try looking for it - it'd yield a near linear factor-of-two speed 
increase.

-Ian

Re: Example idea for how to solve the clock/cnf problem.

[Ian Molton]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

Ooops. Patch attached this time.

Please comment on this. (its a WIP remember, so trivial stuff is to be 
ignored - I wanna get the functionality right first.)

Note in particular the change in the suspend/resume paths - we no longer 
(ab)use the enable/disable hooks, which may break some users of the driver.

I havent decided how I'm going to map the conf area in the MFD drivers 
in a nice way yet. As there are no known devices with TWO of these chips 
in them yet, I may do a static one-off mapping for now. At least this 
will leave tmio-mmc.{c,h} pure and free from all CNF area code.

-Ian


[-- Attachment #2: diffout --]
[-- Type: text/plain, Size: 9228 bytes --]

diff --git a/drivers/mfd/tc6393xb.c b/drivers/mfd/tc6393xb.c
index 1429a73..5db07d7 100644
--- a/drivers/mfd/tc6393xb.c
+++ b/drivers/mfd/tc6393xb.c
@@ -138,6 +138,8 @@ static int tc6393xb_nand_enable(struct platform_device *nand)
 
 static struct tmio_mmc_data tc6393xb_mmc_data = {
 	.hclk = 24000000,
+	.set_pwr = tmio_mmc_pwr,
+	.set_no_clk_div = tmio_mmc_clk_div,
 };
 
 static struct resource __devinitdata tc6393xb_nand_resources[] = {
@@ -346,6 +348,59 @@ int tc6393xb_lcd_mode(struct platform_device *fb,
 }
 EXPORT_SYMBOL(tc6393xb_lcd_mode);
 
+
+#define CNF_CMD     0x04
+#define CNF_CTL_BASE   0x10
+#define CNF_INT_PIN  0x3d
+#define CNF_STOP_CLK_CTL 0x40
+#define CNF_GCLK_CTL 0x41
+#define CNF_SD_CLK_MODE 0x42
+#define CNF_PIN_STATUS 0x44
+#define CNF_PWR_CTL_1 0x48
+#define CNF_PWR_CTL_2 0x49
+#define CNF_PWR_CTL_3 0x4a
+#define CNF_CARD_DETECT_MODE 0x4c
+#define CNF_SD_SLOT 0x50
+#define CNF_EXT_GCLK_CTL_1 0xf0
+#define CNF_EXT_GCLK_CTL_2 0xf1
+#define CNF_EXT_GCLK_CTL_3 0xf9
+#define CNF_SD_LED_EN_1 0xfa
+#define CNF_SD_LED_EN_2 0xfe
+
+#define   SDCREN 0x2   /* Enable access to MMC CTL regs. (flag in COMMAND_REG)*/
+
+
+static int tmio_mmc_enable(struct platform_device *dev) {
+	/* Enable the MMC/SD Control registers */
+	sd_config_write16(host, CNF_CMD, SDCREN);
+	sd_config_write32(host, CNF_CTL_BASE,
+		(tc6393xb_mmc_resources[0].start) & 0xfffe);
+
+	/* Disable SD power during suspend */
+	sd_config_write8(host, CNF_PWR_CTL_3, 0x01);
+
+	/* The below is required but why? FIXME */
+	sd_config_write8(host, CNF_STOP_CLK_CTL, 0x1f);
+
+	/* Power down SD bus*/
+	sd_config_write8(host, CNF_PWR_CTL_2, 0x00);
+}
+
+static int tmio_mmc_resume(struct platform_device *dev) {
+	/* Enable the MMC/SD Control registers */
+	sd_config_write16(host, CNF_CMD, SDCREN);
+	sd_config_write32(host, CNF_CTL_BASE,
+		(dev->resource[0].start >> host->bus_shift) & 0xfffe);
+}
+
+static void tmio_mmc_pwr(struct tmio_mmc_host *host, int state) {
+        sd_config_write8(host, CNF_PWR_CTL_2, state?0x02:0x00);
+}
+
+static void tmio_mmc_clk_div(struct tmio_mmc_host *host, int state) {
+	sd_config_write8(host, CNF_SD_CLK_MODE, state?1:0);
+}
+
 static struct mfd_cell __devinitdata tc6393xb_cells[] = {
 	[TC6393XB_CELL_NAND] = {
 		.name = "tmio-nand",
@@ -355,6 +410,8 @@ static struct mfd_cell __devinitdata tc6393xb_cells[] = {
 	},
 	[TC6393XB_CELL_MMC] = {
 		.name = "tmio-mmc",
+		.enable = tmio_mmc_enable,
+		.resume = tmio_mmc_resume,
 		.driver_data = &tc6393xb_mmc_data,
 		.num_resources = ARRAY_SIZE(tc6393xb_mmc_resources),
 		.resources = tc6393xb_mmc_resources,
diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
index 91991b4..0b6075f 100644
--- a/drivers/mmc/host/tmio_mmc.c
+++ b/drivers/mmc/host/tmio_mmc.c
@@ -46,7 +46,9 @@ static void tmio_mmc_set_clock(struct tmio_mmc_host *host, int new_clock)
 		clk |= 0x100;
 	}
 
-	sd_config_write8(host, CNF_SD_CLK_MODE, clk >> 22);
+	if(host->set_no_clk_div)
+		host->set_no_clk_div(host, (clk>>22) & 1);
+
 	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, clk & 0x1ff);
 }
 
@@ -427,12 +429,13 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	/* Power sequence - OFF -> ON -> UP */
 	switch (ios->power_mode) {
 	case MMC_POWER_OFF: /* power down SD bus */
-		sd_config_write8(host, CNF_PWR_CTL_2, 0x00);
+		if(host->set_pwr)
+			host->set_pwr(host, 0);
 		tmio_mmc_clk_stop(host);
 		break;
 	case MMC_POWER_ON: /* power up SD bus */
-
-		sd_config_write8(host, CNF_PWR_CTL_2, 0x02);
+		if(host->set_pwr)
+			host->set_pwr(host, 1);
 		break;
 	case MMC_POWER_UP: /* start bus clock */
 		tmio_mmc_clk_start(host);
@@ -475,8 +478,8 @@ static int tmio_mmc_suspend(struct platform_device *dev, pm_message_t state)
 	ret = mmc_suspend_host(mmc, state);
 
 	/* Tell MFD core it can disable us now.*/
-	if (!ret && cell->disable)
-		cell->disable(dev);
+	if (!ret && cell->suspend)
+		cell->suspend(dev);
 
 	return ret;
 }
@@ -489,17 +492,12 @@ static int tmio_mmc_resume(struct platform_device *dev)
 	int ret = 0;
 
 	/* Tell the MFD core we are ready to be enabled */
-	if (cell->enable) {
-		ret = cell->enable(dev);
+	if (cell->resume) {
+		ret = cell->resume(dev);
 		if (ret)
 			goto out;
 	}
 
-	/* Enable the MMC/SD Control registers */
-	sd_config_write16(host, CNF_CMD, SDCREN);
-	sd_config_write32(host, CNF_CTL_BASE,
-		(dev->resource[0].start >> host->bus_shift) & 0xfffe);
-
 	mmc_resume_host(mmc);
 
 out:
@@ -514,7 +512,7 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 {
 	struct mfd_cell	*cell = (struct mfd_cell *)dev->dev.platform_data;
 	struct tmio_mmc_data *pdata;
-	struct resource *res_ctl, *res_cnf;
+	struct resource *res_ctl;
 	struct tmio_mmc_host *host;
 	struct mmc_host *mmc;
 	int ret = -EINVAL;
@@ -523,8 +521,7 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 		goto out;
 
 	res_ctl = platform_get_resource(dev, IORESOURCE_MEM, 0);
-	res_cnf = platform_get_resource(dev, IORESOURCE_MEM, 1);
-	if (!res_ctl || !res_cnf)
+	if (!res_ctl)
 		goto out;
 
 	pdata = cell->driver_data;
@@ -541,6 +538,9 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	host->mmc = mmc;
 	platform_set_drvdata(dev, mmc);
 
+	host->set_pwr = pdata->set_pwr;
+	host->set_no_clk_div = pdata->set_no_clk_div;
+
 	/* SD control register space size is 0x200, 0x400 for bus_shift=1 */
 	host->bus_shift = resource_size(res_ctl) >> 10;
 
@@ -548,10 +548,6 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	if (!host->ctl)
 		goto host_free;
 
-	host->cnf = ioremap(res_cnf->start, resource_size(res_cnf));
-	if (!host->cnf)
-		goto unmap_ctl;
-
 	mmc->ops = &tmio_mmc_ops;
 	mmc->caps = MMC_CAP_4_BIT_DATA;
 	mmc->f_max = pdata->hclk;
@@ -562,23 +558,9 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	if (cell->enable) {
 		ret = cell->enable(dev);
 		if (ret)
-			goto unmap_cnf;
+			goto unmap_ctl;
 	}
 
-	/* Enable the MMC/SD Control registers */
-	sd_config_write16(host, CNF_CMD, SDCREN);
-	sd_config_write32(host, CNF_CTL_BASE,
-		(dev->resource[0].start >> host->bus_shift) & 0xfffe);
-
-	/* Disable SD power during suspend */
-	sd_config_write8(host, CNF_PWR_CTL_3, 0x01);
-
-	/* The below is required but why? FIXME */
-	sd_config_write8(host, CNF_STOP_CLK_CTL, 0x1f);
-
-	/* Power down SD bus*/
-	sd_config_write8(host, CNF_PWR_CTL_2, 0x00);
-
 	tmio_mmc_clk_stop(host);
 	reset(host);
 
@@ -586,14 +568,14 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	if (ret >= 0)
 		host->irq = ret;
 	else
-		goto unmap_cnf;
+		goto unmap_ctl;
 
 	disable_mmc_irqs(host, TMIO_MASK_ALL);
 
 	ret = request_irq(host->irq, tmio_mmc_irq, IRQF_DISABLED |
 		IRQF_TRIGGER_FALLING, "tmio-mmc", host);
 	if (ret)
-		goto unmap_cnf;
+		goto unmap_ctl;
 
 	mmc_add_host(mmc);
 
@@ -605,8 +587,6 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 
 	return 0;
 
-unmap_cnf:
-	iounmap(host->cnf);
 unmap_ctl:
 	iounmap(host->ctl);
 host_free:
@@ -626,7 +606,6 @@ static int __devexit tmio_mmc_remove(struct platform_device *dev)
 		mmc_remove_host(mmc);
 		free_irq(host->irq, host);
 		iounmap(host->ctl);
-		iounmap(host->cnf);
 		mmc_free_host(mmc);
 	}
 
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 9fa9985..95d3fc4 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -11,26 +11,6 @@
 
 #include <linux/highmem.h>
 
-#define CNF_CMD     0x04
-#define CNF_CTL_BASE   0x10
-#define CNF_INT_PIN  0x3d
-#define CNF_STOP_CLK_CTL 0x40
-#define CNF_GCLK_CTL 0x41
-#define CNF_SD_CLK_MODE 0x42
-#define CNF_PIN_STATUS 0x44
-#define CNF_PWR_CTL_1 0x48
-#define CNF_PWR_CTL_2 0x49
-#define CNF_PWR_CTL_3 0x4a
-#define CNF_CARD_DETECT_MODE 0x4c
-#define CNF_SD_SLOT 0x50
-#define CNF_EXT_GCLK_CTL_1 0xf0
-#define CNF_EXT_GCLK_CTL_2 0xf1
-#define CNF_EXT_GCLK_CTL_3 0xf9
-#define CNF_SD_LED_EN_1 0xfa
-#define CNF_SD_LED_EN_2 0xfe
-
-#define   SDCREN 0x2   /* Enable access to MMC CTL regs. (flag in COMMAND_REG)*/
-
 #define CTL_SD_CMD 0x00
 #define CTL_ARG_REG 0x04
 #define CTL_STOP_INTERNAL_ACTION 0x08
@@ -163,25 +143,6 @@ static inline void sd_ctrl_write32(struct tmio_mmc_host *host, int addr,
 	writew(val >> 16, host->ctl + ((addr + 2) << host->bus_shift));
 }
 
-static inline void sd_config_write8(struct tmio_mmc_host *host, int addr,
-		u8 val)
-{
-	writeb(val, host->cnf + (addr << host->bus_shift));
-}
-
-static inline void sd_config_write16(struct tmio_mmc_host *host, int addr,
-		u16 val)
-{
-	writew(val, host->cnf + (addr << host->bus_shift));
-}
-
-static inline void sd_config_write32(struct tmio_mmc_host *host, int addr,
-		u32 val)
-{
-	writew(val, host->cnf + (addr << host->bus_shift));
-	writew(val >> 16, host->cnf + ((addr + 2) << host->bus_shift));
-}
-
 #include <linux/scatterlist.h>
 #include <linux/blkdev.h>
 
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index 6b9c5d0..c09f284 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -23,6 +23,8 @@
  */
 struct tmio_mmc_data {
 	const unsigned int		hclk;
+	void (*set_pwr)(struct tmio_mmc_host *host, int state);
+	void (*set_no_clk_div)(struct tmio_mmc_host *host, int state);
 };
 
 /*

Re: MMC: Make the configuration memory resource optional

[Guennadi Liakhovetski]

On Wed, 5 Aug 2009, Ian Molton wrote:

> Guennadi Liakhovetski wrote:
> 
> > How do we verify this? Do I need some "very fast" card, so that sd would try
> > to drive the clock beyond pdata->hclk / 2, which should then fail? 
> 
> One way would be a 'scope on the card pins, but since you havent said I am
> assuming you have not altered the clock-speed setting routine.
> 
> Here you can see why I didnt like the silently-drop-conf-accesses version of
> the patch.  If you had not made conf area accesses silently succeed, you'd
> have found one lurking in the set_clock function.
> 
> Since your controller has no CNF area, it was discarding the write to the 1:1
> clock bit (that isnt  in its none-existant conf area) and thus you got the
> next lowest clock as a result (/2)
> 
> > Currently I'm using 24MHz (copied from the original driver), and the only
> > thing I know about the controller is its "Maximum operating frequency: 25
> > MHz."
> 
> Thats the HCLK frequency, not the card clock.

Sure, and the card clock is derived from the HCLK, as you describe below.

> the TMIO MFD devices can divide this HCLK by anything from 512 to 2, and  MFD
> devices have a facility to disable the divider, yielding full HCLK speed as
> the card clock.
> 
> If you havent already found a 1:1 clock enable bit on your device, you might
> try looking for it - it'd yield a near linear factor-of-two speed increase.

I think, even getting the driver work with half the maximum speed (without 
the 1:1 speed) would be a good progress for SH. And I personally have no 
idea how I could "find" that divider-disable bit. Please notice, that I'm 
away for a week starting tomorrow, hopefully, Magnus will be able to 
further work with you on this during this time.

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

Re: MMC: Make the configuration memory resource optional

[Ian Molton]

Guennadi Liakhovetski wrote:

>> Thats the HCLK frequency, not the card clock.
> 
> Sure, and the card clock is derived from the HCLK, as you describe below.

Ok, so then your controller (unless it uses a different scheme 
altogether for the card clock divider) probably has a max card clock of 
12MHz and a min of 24/512MHz.

I will change the driver so that it automatically alters the max 
frequency based on the presence of a function to (en,dis)able the 1:1 bit.

> I think, even getting the driver work with half the maximum speed (without 
> the 1:1 speed) would be a good progress for SH.

I'm fine with that. If someone who _has_ this hardware could get a 
'scope on the card clock and see what frequencies actually appear out 
there, I'd _really_ appreciate it.

In the meantime, I'm going to assume it follows the scheme above.

> And I personally have no 
> idea how I could "find" that divider-disable bit.

Just how sure are you that your chip _doesnt_ have the cnf area, before 
we go to far on this... how did you arrive at that conclusion?

Have you checked to see if it appears at other offsets than it does in 
the MFD chips ?

> Please notice, that I'm 
> away for a week starting tomorrow, hopefully, Magnus will be able to 
> further work with you on this during this time.

Hope so - It was aloways my goal to make tmio-mmc reuseable - thats why 
I pushed for the MFD framework in the first place...

-Ian

MMC / MFD / Clocks

[Ian Molton]

[-- Attachment #1: Type: text/plain, Size: 270 bytes --]

Hi guys,

I've done a bit more on the clocking code - it compiles now, but the mmc 
driver needs to pass some more data out than it does right now before 
the code will actually work.

Unless I hear any other comments, this is the direction the driver will 
take.

-Ian

[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 11265 bytes --]

diff --git a/drivers/mfd/tc6393xb.c b/drivers/mfd/tc6393xb.c
index 1429a73..e5235d6 100644
--- a/drivers/mfd/tc6393xb.c
+++ b/drivers/mfd/tc6393xb.c
@@ -136,10 +136,6 @@ static int tc6393xb_nand_enable(struct platform_device *nand)
 	return 0;
 }
 
-static struct tmio_mmc_data tc6393xb_mmc_data = {
-	.hclk = 24000000,
-};
-
 static struct resource __devinitdata tc6393xb_nand_resources[] = {
 	{
 		.start	= 0x1000,
@@ -164,11 +160,11 @@ static struct resource __devinitdata tc6393xb_mmc_resources[] = {
 		.end	= 0x9ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	{
-		.start	= 0x200,
-		.end	= 0x2ff,
-		.flags	= IORESOURCE_MEM,
-	},
+//	{
+//		.start	= 0x200,
+//		.end	= 0x2ff,
+//		.flags	= IORESOURCE_MEM,
+//	},
 	{
 		.start	= IRQ_TC6393_MMC,
 		.end	= IRQ_TC6393_MMC,
@@ -346,6 +342,84 @@ int tc6393xb_lcd_mode(struct platform_device *fb,
 }
 EXPORT_SYMBOL(tc6393xb_lcd_mode);
 
+
+#define CNF_CMD     0x04
+#define CNF_CTL_BASE   0x10
+#define CNF_INT_PIN  0x3d
+#define CNF_STOP_CLK_CTL 0x40
+#define CNF_GCLK_CTL 0x41
+#define CNF_SD_CLK_MODE 0x42
+#define CNF_PIN_STATUS 0x44
+#define CNF_PWR_CTL_1 0x48
+#define CNF_PWR_CTL_2 0x49
+#define CNF_PWR_CTL_3 0x4a
+#define CNF_CARD_DETECT_MODE 0x4c
+#define CNF_SD_SLOT 0x50
+#define CNF_EXT_GCLK_CTL_1 0xf0
+#define CNF_EXT_GCLK_CTL_2 0xf1
+#define CNF_EXT_GCLK_CTL_3 0xf9
+#define CNF_SD_LED_EN_1 0xfa
+#define CNF_SD_LED_EN_2 0xfe
+
+#define   SDCREN 0x2   /* Enable access to MMC CTL regs. (flag in COMMAND_REG)*/
+
+#define sd_config_write8(a, b)  tmio_iowrite8((b), (a) + 0x200)
+#define sd_config_write16(a, b)  tmio_iowrite16((b), (a) + 0x200)
+#define sd_config_write32(a, b)  tmio_iowrite32((b), (a) + 0x200)
+
+static int tmio_mmc_enable(struct platform_device *mmc) {
+	struct platform_device *dev = to_platform_device(mmc->dev.parent);
+        struct tc6393xb *tc6393xb = platform_get_drvdata(dev);
+
+	/* Enable the MMC/SD Control registers */
+	sd_config_write16(tc6393xb->scr + CNF_CMD, SDCREN);
+	sd_config_write32(tc6393xb->scr + CNF_CTL_BASE,
+		(tc6393xb_mmc_resources[0].start) & 0xfffe);
+
+	/* Disable SD power during suspend */
+	sd_config_write8(tc6393xb->scr + CNF_PWR_CTL_3, 0x01);
+
+	/* The below is required but why? FIXME */
+	sd_config_write8(tc6393xb->scr + CNF_STOP_CLK_CTL, 0x1f);
+
+	/* Power down SD bus*/
+	sd_config_write8(tc6393xb->scr + CNF_PWR_CTL_2, 0x00);
+
+	return 0;
+}
+
+static int tmio_mmc_resume(struct platform_device *mmc) {
+        struct platform_device *dev = to_platform_device(mmc->dev.parent);
+        struct tc6393xb *tc6393xb = platform_get_drvdata(dev);
+
+	/* Enable the MMC/SD Control registers */
+	sd_config_write16(tc6393xb->scr + CNF_CMD, SDCREN);
+	sd_config_write32(tc6393xb->scr + CNF_CTL_BASE,
+		(tc6393xb_mmc_resources[0].start) & 0xfffe);
+
+	return 0;
+}
+
+static void tmio_mmc_pwr(struct platform_device *mmc, int state) {
+        struct platform_device *dev = to_platform_device(mmc->dev.parent);
+        struct tc6393xb *tc6393xb = platform_get_drvdata(dev);
+
+        sd_config_write8(tc6393xb->scr + CNF_PWR_CTL_2, state?0x02:0x00);
+}
+
+static void tmio_mmc_clk_div(struct platform_device *mmc, int state) {
+        struct platform_device *dev = to_platform_device(mmc->dev.parent);
+        struct tc6393xb *tc6393xb = platform_get_drvdata(dev);
+
+	sd_config_write8(tc6393xb->scr + CNF_SD_CLK_MODE, state?1:0);
+}
+
+static struct tmio_mmc_data tc6393xb_mmc_data = {
+	.hclk = 24000000,
+	.set_pwr = tmio_mmc_pwr,
+	.set_no_clk_div = tmio_mmc_clk_div,
+};
+
 static struct mfd_cell __devinitdata tc6393xb_cells[] = {
 	[TC6393XB_CELL_NAND] = {
 		.name = "tmio-nand",
@@ -355,6 +429,8 @@ static struct mfd_cell __devinitdata tc6393xb_cells[] = {
 	},
 	[TC6393XB_CELL_MMC] = {
 		.name = "tmio-mmc",
+		.enable = tmio_mmc_enable,
+		.resume = tmio_mmc_resume,
 		.driver_data = &tc6393xb_mmc_data,
 		.num_resources = ARRAY_SIZE(tc6393xb_mmc_resources),
 		.resources = tc6393xb_mmc_resources,
diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
index 91991b4..2045ba5 100644
--- a/drivers/mmc/host/tmio_mmc.c
+++ b/drivers/mmc/host/tmio_mmc.c
@@ -46,7 +46,9 @@ static void tmio_mmc_set_clock(struct tmio_mmc_host *host, int new_clock)
 		clk |= 0x100;
 	}
 
-	sd_config_write8(host, CNF_SD_CLK_MODE, clk >> 22);
+	if(host->set_no_clk_div)
+		host->set_no_clk_div(NULL, (clk>>22) & 1);
+
 	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, clk & 0x1ff);
 }
 
@@ -427,12 +429,13 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	/* Power sequence - OFF -> ON -> UP */
 	switch (ios->power_mode) {
 	case MMC_POWER_OFF: /* power down SD bus */
-		sd_config_write8(host, CNF_PWR_CTL_2, 0x00);
+		if(host->set_pwr)
+			host->set_pwr(NULL, 0);
 		tmio_mmc_clk_stop(host);
 		break;
 	case MMC_POWER_ON: /* power up SD bus */
-
-		sd_config_write8(host, CNF_PWR_CTL_2, 0x02);
+		if(host->set_pwr)
+			host->set_pwr(NULL, 1);
 		break;
 	case MMC_POWER_UP: /* start bus clock */
 		tmio_mmc_clk_start(host);
@@ -475,8 +478,8 @@ static int tmio_mmc_suspend(struct platform_device *dev, pm_message_t state)
 	ret = mmc_suspend_host(mmc, state);
 
 	/* Tell MFD core it can disable us now.*/
-	if (!ret && cell->disable)
-		cell->disable(dev);
+	if (!ret && cell->suspend)
+		cell->suspend(dev);
 
 	return ret;
 }
@@ -485,21 +488,15 @@ static int tmio_mmc_resume(struct platform_device *dev)
 {
 	struct mfd_cell	*cell = (struct mfd_cell *)dev->dev.platform_data;
 	struct mmc_host *mmc = platform_get_drvdata(dev);
-	struct tmio_mmc_host *host = mmc_priv(mmc);
 	int ret = 0;
 
 	/* Tell the MFD core we are ready to be enabled */
-	if (cell->enable) {
-		ret = cell->enable(dev);
+	if (cell->resume) {
+		ret = cell->resume(dev);
 		if (ret)
 			goto out;
 	}
 
-	/* Enable the MMC/SD Control registers */
-	sd_config_write16(host, CNF_CMD, SDCREN);
-	sd_config_write32(host, CNF_CTL_BASE,
-		(dev->resource[0].start >> host->bus_shift) & 0xfffe);
-
 	mmc_resume_host(mmc);
 
 out:
@@ -514,7 +511,7 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 {
 	struct mfd_cell	*cell = (struct mfd_cell *)dev->dev.platform_data;
 	struct tmio_mmc_data *pdata;
-	struct resource *res_ctl, *res_cnf;
+	struct resource *res_ctl;
 	struct tmio_mmc_host *host;
 	struct mmc_host *mmc;
 	int ret = -EINVAL;
@@ -523,8 +520,7 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 		goto out;
 
 	res_ctl = platform_get_resource(dev, IORESOURCE_MEM, 0);
-	res_cnf = platform_get_resource(dev, IORESOURCE_MEM, 1);
-	if (!res_ctl || !res_cnf)
+	if (!res_ctl)
 		goto out;
 
 	pdata = cell->driver_data;
@@ -541,6 +537,9 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	host->mmc = mmc;
 	platform_set_drvdata(dev, mmc);
 
+	host->set_pwr = pdata->set_pwr;
+	host->set_no_clk_div = pdata->set_no_clk_div;
+
 	/* SD control register space size is 0x200, 0x400 for bus_shift=1 */
 	host->bus_shift = resource_size(res_ctl) >> 10;
 
@@ -548,10 +547,6 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	if (!host->ctl)
 		goto host_free;
 
-	host->cnf = ioremap(res_cnf->start, resource_size(res_cnf));
-	if (!host->cnf)
-		goto unmap_ctl;
-
 	mmc->ops = &tmio_mmc_ops;
 	mmc->caps = MMC_CAP_4_BIT_DATA;
 	mmc->f_max = pdata->hclk;
@@ -562,23 +557,9 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	if (cell->enable) {
 		ret = cell->enable(dev);
 		if (ret)
-			goto unmap_cnf;
+			goto unmap_ctl;
 	}
 
-	/* Enable the MMC/SD Control registers */
-	sd_config_write16(host, CNF_CMD, SDCREN);
-	sd_config_write32(host, CNF_CTL_BASE,
-		(dev->resource[0].start >> host->bus_shift) & 0xfffe);
-
-	/* Disable SD power during suspend */
-	sd_config_write8(host, CNF_PWR_CTL_3, 0x01);
-
-	/* The below is required but why? FIXME */
-	sd_config_write8(host, CNF_STOP_CLK_CTL, 0x1f);
-
-	/* Power down SD bus*/
-	sd_config_write8(host, CNF_PWR_CTL_2, 0x00);
-
 	tmio_mmc_clk_stop(host);
 	reset(host);
 
@@ -586,14 +567,14 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	if (ret >= 0)
 		host->irq = ret;
 	else
-		goto unmap_cnf;
+		goto unmap_ctl;
 
 	disable_mmc_irqs(host, TMIO_MASK_ALL);
 
 	ret = request_irq(host->irq, tmio_mmc_irq, IRQF_DISABLED |
 		IRQF_TRIGGER_FALLING, "tmio-mmc", host);
 	if (ret)
-		goto unmap_cnf;
+		goto unmap_ctl;
 
 	mmc_add_host(mmc);
 
@@ -605,8 +586,6 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 
 	return 0;
 
-unmap_cnf:
-	iounmap(host->cnf);
 unmap_ctl:
 	iounmap(host->ctl);
 host_free:
@@ -626,7 +605,6 @@ static int __devexit tmio_mmc_remove(struct platform_device *dev)
 		mmc_remove_host(mmc);
 		free_irq(host->irq, host);
 		iounmap(host->ctl);
-		iounmap(host->cnf);
 		mmc_free_host(mmc);
 	}
 
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 9fa9985..d7bc12d 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -11,26 +11,6 @@
 
 #include <linux/highmem.h>
 
-#define CNF_CMD     0x04
-#define CNF_CTL_BASE   0x10
-#define CNF_INT_PIN  0x3d
-#define CNF_STOP_CLK_CTL 0x40
-#define CNF_GCLK_CTL 0x41
-#define CNF_SD_CLK_MODE 0x42
-#define CNF_PIN_STATUS 0x44
-#define CNF_PWR_CTL_1 0x48
-#define CNF_PWR_CTL_2 0x49
-#define CNF_PWR_CTL_3 0x4a
-#define CNF_CARD_DETECT_MODE 0x4c
-#define CNF_SD_SLOT 0x50
-#define CNF_EXT_GCLK_CTL_1 0xf0
-#define CNF_EXT_GCLK_CTL_2 0xf1
-#define CNF_EXT_GCLK_CTL_3 0xf9
-#define CNF_SD_LED_EN_1 0xfa
-#define CNF_SD_LED_EN_2 0xfe
-
-#define   SDCREN 0x2   /* Enable access to MMC CTL regs. (flag in COMMAND_REG)*/
-
 #define CTL_SD_CMD 0x00
 #define CTL_ARG_REG 0x04
 #define CTL_STOP_INTERNAL_ACTION 0x08
@@ -110,7 +90,6 @@
 
 
 struct tmio_mmc_host {
-	void __iomem *cnf;
 	void __iomem *ctl;
 	unsigned long bus_shift;
 	struct mmc_command      *cmd;
@@ -119,6 +98,10 @@ struct tmio_mmc_host {
 	struct mmc_host         *mmc;
 	int                     irq;
 
+	/* Callbacks for clock / power control */
+	void (*set_pwr)(struct platform_device *host, int state);
+	void (*set_no_clk_div)(struct platform_device *host, int state);
+
 	/* pio related stuff */
 	struct scatterlist      *sg_ptr;
 	unsigned int            sg_len;
@@ -163,25 +146,6 @@ static inline void sd_ctrl_write32(struct tmio_mmc_host *host, int addr,
 	writew(val >> 16, host->ctl + ((addr + 2) << host->bus_shift));
 }
 
-static inline void sd_config_write8(struct tmio_mmc_host *host, int addr,
-		u8 val)
-{
-	writeb(val, host->cnf + (addr << host->bus_shift));
-}
-
-static inline void sd_config_write16(struct tmio_mmc_host *host, int addr,
-		u16 val)
-{
-	writew(val, host->cnf + (addr << host->bus_shift));
-}
-
-static inline void sd_config_write32(struct tmio_mmc_host *host, int addr,
-		u32 val)
-{
-	writew(val, host->cnf + (addr << host->bus_shift));
-	writew(val >> 16, host->cnf + ((addr + 2) << host->bus_shift));
-}
-
 #include <linux/scatterlist.h>
 #include <linux/blkdev.h>
 
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index 6b9c5d0..cb34c28 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -23,6 +23,8 @@
  */
 struct tmio_mmc_data {
 	const unsigned int		hclk;
+	void (*set_pwr)(struct platform_device *host, int state);
+	void (*set_no_clk_div)(struct platform_device *host, int state);
 };
 
 /*

Re: MMC / MFD / Clocks

[Magnus Damm]

Hi Ian,

Thanks for your work on this!

On Mon, Aug 10, 2009 at 4:10 AM, Ian Molton<ian@mnementh.co.uk> wrote:
> Hi guys,
>
> I've done a bit more on the clocking code - it compiles now, but the mmc
> driver needs to pass some more data out than it does right now before the
> code will actually work.

Looking good! I will most likely use NULL for ->set_pwr() and
->set_no_clk_div() and from that perspective the patch looks not so
far from complete already.

> Unless I hear any other comments, this is the direction the driver will
> take.

Good direction!

The office is closed this week due to holidays, but after that i'll
dust off my little Fluke Scopemeter and measure the frequency. Not
sure if there is any divisor that modifies the input clock for the
hardware block before we get a change to control it using
CTL_SD_CARD_CLK_CTL.

Anyway, thanks a lot for your help. Please CC this gmail address when
you update the patch.

Cheers,

/ magnus

Re: MMC: Make the configuration memory resource optional

[Magnus Damm]

[-- Attachment #1: Type: text/plain, Size: 1983 bytes --]

Hi Ian!

On Thu, Aug 6, 2009 at 8:06 AM, Ian Molton<ian@mnementh.co.uk> wrote:
> Ok, so then your controller (unless it uses a different scheme altogether
> for the card clock divider) probably has a max card clock of 12MHz and a min
> of 24/512MHz.

Allright, I've now tested the tmio_mmc driver on a ms7724se board. In
the current configuration the tmio_mmc block is driven by a 83 Mhz
clock. We can change the frequency of this clock but it will affect
other hardware blocks as well so I'd rather not unless I really have
to.

> I'm fine with that. If someone who _has_ this hardware could get a 'scope on
> the card clock and see what frequencies actually appear out there, I'd
> _really_ appreciate it.

I've done some measurements with my Fluke 123 Scopemeter. It's limited
to 20 MHz so I didn't measure all combinations. I hooked it up to pin
5 of CN7 on the ms7724se board. In total I did 7 measurements, with
XSHIFT from 0 to 6. One line per measurement below. The number on the
far right is the value from the scope.

using clock 162760 (162760, 83333332) [162760] 0x0180 -> 162.7 kHz
using clock 325520 (162760, 83333332) [325520] 0x0140 -> 0.326 MHz
using clock 651040 (162760, 83333332) [651040] 0x0120 -> 0.651 MHz
using clock 1302080 (162760, 83333332) [1302080] 0x0110 -> 01.30 MHz
using clock 2604160 (162760, 83333332) [2604160] 0x0108 -> 02.61 MHz
using clock 5208320 (162760, 83333332) [5208320] 0x0104 -> 05.21 MHz
using clock 10416640 (162760, 83333332) [10416640] 0x0102 -> 10.42 MHz

Everything seems to work as expected what I can tell. The frequencies match.

I've attached a patch which shows details of the printouts. Hopefully
it includes all you need.

FYI, I needed to modify the tmio_mmc driver to change the
dev->num_resources check in probe from 3 to 2 to support the SuperH
hardware. Can you please roll in that change and send an updated
version of your "MMC / MFD / Clocks" patch whenever you have time?

Thanks for your help!

Cheers,

/ magnus

[-- Attachment #2: linux-2.6.32-pre-sh-se7724-tmio-measure-hack-20090818.patch --]
[-- Type: application/octet-stream, Size: 863 bytes --]

--- 0007/drivers/mmc/host/tmio_mmc.c
+++ work/drivers/mmc/host/tmio_mmc.c	2009-08-18 17:03:22.000000000 +0900
@@ -35,13 +35,16 @@
 
 #include "tmio_mmc.h"
 
+#define XSHIFT 6
+#define XLIMIT (162760 << XSHIFT)
+
 static void tmio_mmc_set_clock(struct tmio_mmc_host *host, int new_clock)
 {
 	u32 clk = 0, clock;
 
 	if (new_clock) {
 		for (clock = host->mmc->f_min, clk = 0x80000080;
-			new_clock >= (clock<<1); clk >>= 1)
+		     (clock < XLIMIT) && (new_clock >= (clock<<1)); clk >>= 1)
 			clock <<= 1;
 		clk |= 0x100;
 	}
@@ -49,6 +52,10 @@ static void tmio_mmc_set_clock(struct tm
 	if(host->set_no_clk_div)
 		host->set_no_clk_div(NULL, (clk>>22) & 1);
 
+	printk("using clock %d (%d, %d) [%d] 0x%04x\n",
+	       clock, host->mmc->f_min, host->mmc->f_max, XLIMIT,
+	       clk & 0x1ff);
+
 	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, clk & 0x1ff);
 }
 

Re: Example idea for how to solve the clock/cnf problem.

[Magnus Damm]

Hi Ian,

On Thu, Aug 6, 2009 at 7:43 AM, Ian Molton<ian@mnementh.co.uk> wrote:
> Ooops. Patch attached this time.
>
> Please comment on this. (its a WIP remember, so trivial stuff is to be
> ignored - I wanna get the functionality right first.)
>
> Note in particular the change in the suspend/resume paths - we no longer
> (ab)use the enable/disable hooks, which may break some users of the driver.
>
> I havent decided how I'm going to map the conf area in the MFD drivers in a
> nice way yet. As there are no known devices with TWO of these chips in them
> yet, I may do a static one-off mapping for now. At least this will leave
> tmio-mmc.{c,h} pure and free from all CNF area code.

Thanks for your work on this. There is a number of resources check
that is still fixed at 3 instead of 2, but apart from that all is ok
from the SuperH side of things.

In case you missed it, please see my earlier comments and measurements
in this email:

http://lkml.org/lkml/2009/8/18/67

Do you have any updates queued up for this patch, or do you want me to
submit my fix on top of this one?

Cheers,

/ magnus