LinuxLists.cc - MMC: Make the configuration memory resource optional

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

On Fri, Jul 17, 2009 at 8:10 PM, Guennadi
Liakhovetski<[email protected]> 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 <[email protected]>
> CC: Magnus Damm <[email protected]>
> ---
> 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

2009-07-17 14:33:50

by Guennadi Liakhovetski

[permalink] [raw]

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

(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<[email protected]> 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 <[email protected]>
> > CC: Magnus Damm <[email protected]>
> > ---
> > 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/

2009-07-17 17:38:32

[permalink] [raw]

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

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<[email protected]> 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.

:-)

2009-07-23 10:29:10

[permalink] [raw]

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

[added akpm to the CC]

On Fri, Jul 17, 2009 at 11:34 PM, Guennadi
Liakhovetski<[email protected]> 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<[email protected]> 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 <[email protected]>

>> 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

2009-07-28 13:55:18

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

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

2009-07-29 02:48:53

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

Hi Ian,

On Tue, Jul 28, 2009 at 10:55 PM, Ian Molton<[email protected]> 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

2009-07-29 07:31:03

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

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.

2009-07-29 10:24:29

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

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

2009-07-29 11:58:20

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

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.

2009-07-29 12:27:55

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

On Wed, Jul 29, 2009 at 8:58 PM, Mark
Brown<[email protected]> 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

2009-07-29 12:35:53

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

On Wed, Jul 29, 2009 at 09:27:54PM +0900, Magnus Damm wrote:
> On Wed, Jul 29, 2009 at 8:58 PM, Mark
> Brown<[email protected]> 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.

2009-07-29 12:37:40

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

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

2009-07-29 12:42:35

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

On Wed, Jul 29, 2009 at 09:27:54PM +0900, Magnus Damm wrote:
> On Wed, Jul 29, 2009 at 8:58 PM, Mark
> Brown<[email protected]> 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.

2009-07-29 12:51:31

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

On Wed, Jul 29, 2009 at 9:42 PM, Mark
Brown<[email protected]> 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<[email protected]> 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

2009-07-29 12:58:46

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

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...

2009-07-29 12:59:50

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

On Wed, Jul 29, 2009 at 09:51:30PM +0900, Magnus Damm wrote:
> On Wed, Jul 29, 2009 at 9:42 PM, Mark
> Brown<[email protected]> 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.

2009-07-29 13:08:43

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

On Wed, Jul 29, 2009 at 9:58 PM, Ian Molton<[email protected]> 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

2009-07-29 13:11:56

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

On 29 Jul 2009, at 13:58, Ian Molton <[email protected]> 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.

2009-07-29 13:51:52

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

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)

2009-07-29 20:17:09

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

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.

2009-07-29 20:55:34

by Philipp Zabel

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

On Wed, Jul 29, 2009 at 10:17 PM, Paul Mundt<[email protected]> 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

2009-07-29 21:03:22

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

On Wed, Jul 29, 2009 at 10:55:31PM +0200, pHilipp Zabel wrote:
> On Wed, Jul 29, 2009 at 10:17 PM, Paul Mundt<[email protected]> 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!

2009-07-30 10:00:09

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

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

2009-07-30 10:56:31

by Guennadi Liakhovetski

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

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/

2009-07-30 19:21:20

[permalink] [raw]

Subject: Re: MMC: Make the configuration memory resource optional

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

2009-07-30 19:33:56