Re: Fwd: [PATCH] [MMC] fix clock problem in PXA255/270

Previous thread: [PATCH] x86: mtrr cleanup for converting continuous to discrete layout by Yinghai Lu on Sunday, April 27, 2008 - 11:37 pm. (83 messages)

Next thread: [ANNOUNCE] kvm-67 release by Avi Kivity on Monday, April 28, 2008 - 1:56 am. (1 message)
From: Tadeusz Gozdek
Date: Monday, April 28, 2008 - 12:32 am

It solves the problems with clock in the PXA255/270 MMC/SD hardware.
This solution was testes with our devices and works good.
Kernel 2.6.24.4 (but should work with >=2.6.19)

Regards
Tadeusz Gozdek
 
diff -NuarBb /x/work/navi_os/mmc/host/pxamci.c mmc/host/pxamci.c
--- /x/work/navi_os/mmc/host/pxamci.c	2008-03-24 19:49:18.000000000 +0100
+++ mmc/host/pxamci.c	2008-04-25 12:14:00.000000000 +0200
@@ -26,6 +26,7 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/mmc/host.h>
+#include <linux/mmc/mmc.h>
 
 #include <asm/dma.h>
 #include <asm/io.h>
@@ -67,6 +68,8 @@
 	unsigned int		dma_dir;
 };
 
+static int local_cmd = -1;
+
 static void pxamci_stop_clock(struct pxamci_host *host)
 {
 	if (readl(host->base + MMC_STAT) & STAT_CLK_EN) {
@@ -84,6 +87,7 @@
 
 		if (v & STAT_CLK_EN)
 			dev_err(mmc_dev(host->mmc), "unable to stop clock\n");
+		local_cmd = -1;
 	}
 }
 
@@ -232,6 +235,14 @@
 		v = w2;
 	}
 
+	local_cmd = cmd->opcode;
+	if ((local_cmd == MMC_GO_IDLE_STATE) ||
+	    (local_cmd == MMC_GO_INACTIVE_STATE) ||
+	    (local_cmd == MMC_SEND_STATUS) ||
+	    (local_cmd == MMC_STOP_TRANSMISSION) ||
+	    (local_cmd == MMC_SET_BLOCKLEN)
+	    ) pxamci_stop_clock(host);
+
 	if (stat & STAT_TIME_OUT_RESPONSE) {
 		cmd->error = -ETIMEDOUT;
 	} else if (stat & STAT_RES_CRC_ERR && cmd->flags & MMC_RSP_CRC) {
@@ -290,7 +301,7 @@
 
 	host->data = NULL;
 	if (host->mrq->stop) {
-		pxamci_stop_clock(host);
+		//pxamci_stop_clock(host);
 		pxamci_start_cmd(host, host->mrq->stop, host->cmdat);
 	} else {
 		pxamci_finish_request(host, host->mrq);


--

From: Pierre Ossman
Date: Monday, April 28, 2008 - 6:28 am

On Mon, 28 Apr 2008 09:32:49 +0200

What issue is this supposed to solve?

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org
--

From: Tadeusz Gozdek
Date: Monday, April 28, 2008 - 11:49 pm

Hi
As you can check, after finishing all operations clock is leaved as switched on (its switched off and on again during sending next command e.g. GO_INACTIVE_STATE). The same is after wake-up the system, even if the clock was switched off. 
Switched on clock has sometimes an influence on the rest of the hardware e.g. the GPS modules and on battery life in a battery powered systems.

---- Wiadomość Oryginalna ----
Od: Pierre Ossman <drzeus-list@drzeus.cx>
Do: Tadeusz Gozdek <linux_fan@o2.pl>
Kopia do: linux-kernel@vger.kernel.org, Marcel Holtmann <marcel@holtmann.org>
Data: 28 kwietnia 2008 15:28

--

From: Pierre Ossman
Date: Tuesday, April 29, 2008 - 2:39 am

On Tue, 29 Apr 2008 08:49:17 +0200

That is a policy with possible side-effects (i.e. needs to be
configurable), and host independent so it should be in the core. So I'm
afraid I have to NAK your patch.

As a side-note, the PXA driver shouldn't be fiddling with the clock the
way it does right now in the first place, but I have no maintainer to
yell at. Patches removing that behaviour are very welcome.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org
--

From: Tadeusz Gozdek
Date: Tuesday, April 29, 2008 - 3:30 am

OK probably I didn't understand your policy. I ever thought that the host controller should know, when the clock should be off.
The PXA controller is different and it was the simples (but effective) way to solve this difference.

So, I hope it will be usable for people which has problem with "always on" clock, until you solve it in different way (if you going to do this).

Thanks 

--

From: Pierre Ossman
Date: Tuesday, April 29, 2008 - 9:22 am

On Tue, 29 Apr 2008 12:30:44 +0200

No, it's not because it's not my policy, but because the kernel tries
to avoid handling "policy". :)

It is common to talk about software that implements "policy" or
"mechanism". "mechanism" is simple the act of performing a given, well
defined task (e.g. turn on LED A). "policy" on the other hand is
selecting an action when there are multiple, equally valid actions.
Usually this involves the user, directly or indirectly.

The kernel tries to stay clear of "policy" as it is a complex problem
that is better solved per system, in user space. So what I meant when I
said that this is policy, I was saying that this is a "policy" kind of

Different in what regard? Or are you referring to the already present

This is a project that has been popping up now and then, but the big
thing that is lacking is someone who is able to actually measure if
there is a power gain from this. I don't want to add a lot of code that
isn't doing any good, so I want hard numbers.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org
--

From: Tadeusz Gozdek
Date: Wednesday, April 30, 2008 - 1:43 am

I mean already present clock... but it looks only I have problem with this using Linux. 
OK OK. I understand. :)
I check some devices with the SD and Linux inside and all of them had modified driver which is able to switch off the clock :)
Thus the "persistent clock" problem exist, but nobody say about it :)

Thank you for all explanations.
--

Previous thread: [PATCH] x86: mtrr cleanup for converting continuous to discrete layout by Yinghai Lu on Sunday, April 27, 2008 - 11:37 pm. (83 messages)

Next thread: [ANNOUNCE] kvm-67 release by Avi Kivity on Monday, April 28, 2008 - 1:56 am. (1 message)