SDHCI support for Samsung SoC

Some controllers do not provide clock information in their capabilities (in the Samsung case, it is because there are multiple clock sources available to the controller). Add hooks to allow the system to supply clock information.

Signed-off-by Ben Dooks <[email protected]>

If using ADMA, then we should print the ADMA error and current pointer in sdhci_dumpregs() when any debug is requested.

Signed-off-by Ben Dooks <[email protected]>

Add a set_ios hook which is called when the SDHCI driver is called to change parameters such as clock or card width.

Signed-off-by Ben Dooks <[email protected]>

The Samsung SDHCI controller block seems to fail to generate an INT_DATA_END after the transfer has completed and the bus busy state finished.

Changes in e809517f6fa5803a5a1cd56026f0e2190fc13d5c to use the new busy method are the cause of the behaviour change.

Signed-off-by Ben Dooks <[email protected]>

Index: linux.git/drivers/mmc/host/sdhci.c
===================================================================

drivers/mmc/host/sdhci.c +0 -0

@@ -1291,11 +1291,24 @@ static void sdhci_cmd_irq(struct sdhci_h
 	 *       controllers.

Quoted 20 lines · On Mon, 03 Nov 2008 20:09:48 +0000 Ben Dooks <[email protected]> wrote:

The Samsung SDHCI controller block seems to fail to generate an INT_DATA_END after the transfer has completed and the bus busy state finished.

Changes in e809517f6fa5803a5a1cd56026f0e2190fc13d5c to use the new busy method are the cause of the behaviour change.

Signed-off-by Ben Dooks <[email protected]>

Index: linux.git/drivers/mmc/host/sdhci.c
===================================================================

drivers/mmc/host/sdhci.c +2 -0

@@ -1291,11 +1291,24 @@ static void sdhci_cmd_irq(struct sdhci_h
 	 *       controllers.
 	 */
 	if (host->cmd->flags & MMC_RSP_BUSY) {
+		u32 present;
+
 		if (host->cmd->data)

drivers/mmc/host/sdhci.c static void sdhci_cmd_irq(struct sdhci_h · line 1300

Quoted 5 lines

drivers/mmc/host/sdhci.c +1 -1

 			DBG("Cannot wait for busy signal when also "
 				"doing a data transfer");
-		else
+		else if (!(host->quirks & SDHCI_QUIRK_NO_TCIRQ_ON_NOT_BUSY))
 			return;

Not the clearest naming I've ever seen. :)
How about NO_BUSY_IRQ?

drivers/mmc/host/sdhci.c static void sdhci_cmd_irq(struct sdhci_h · line 1306

Quoted 6 lines

drivers/mmc/host/sdhci.c +6 -0

+
+		/* The Samsung SDHCI does not seem to provide an INT_DATA_END
+		 * when the system goes non-busy, so check the state of the
+		 * transfer by reading SDHCI_PRESENT_STATE to see if the
+		 * controller is ready
+		 */

There is already a note about this being a problem earlier up, so I don't think we need another.

drivers/mmc/host/sdhci.c static void sdhci_cmd_irq(struct sdhci_h · line 1310

Quoted 4 lines

drivers/mmc/host/sdhci.c +4 -0

+
+		present = readl(host->ioaddr + SDHCI_PRESENT_STATE);
+		DBG("busy? present %08x, intstat %08x\n", present, intmask);
+

And what does this add really? Controllers not being able to wait for the busy state to end is so common that we can just ignore the problem here.

drivers/mmc/host/sdhci.c static void sdhci_cmd_irq(struct sdhci_h · line 1311

Quoted 1 line

drivers/mmc/host/sdhci.c +1 -0

+#define  SDHCI_DATA_BIT(x)	(1 << ((x) + 20))

This was not supposed to be included I suppose :)

drivers/mmc/host/sdhci-s3c.c new file +4 -0

+static void sdhci_s3c_sel_sclk(struct sdhci_host *host)
+{
+	struct sdhci_s3c *ourhost = to_s3c(host);
+

Quoted 4 lines · On Mon, 03 Nov 2008 20:09:49 +0000 Ben Dooks <[email protected]> wrote:

drivers/mmc/host/sdhci-s3c.c new file +4 -0

+static void sdhci_s3c_sel_sclk(struct sdhci_host *host)
+{
+	struct sdhci_s3c *ourhost = to_s3c(host);
+

drivers/mmc/host/sdhci-s3c.c line 58

Quoted 10 lines

drivers/mmc/host/sdhci-s3c.c new file +10 -0

+	/* select sclk */
+	u32 tmp = readl(host->ioaddr + 0x80);
+
+	if ((tmp & (3 << 4)) == (2 << 4))
+		return;
+
+	tmp &= ~(3<<4);
+	tmp |= (2 << 4);
+	writel(tmp, host->ioaddr + 0x80);
+}

No defines for this? This is not terribly readable.

drivers/mmc/host/sdhci-s3c.c line 110

Quoted 3 lines

drivers/mmc/host/sdhci-s3c.c new file +3 -0

+	if (pdata->cfg_card)
+		pdata->cfg_card(ourhost->pdev, host->ioaddr,
+				ios, host->mmc->card);

What's the deal here? Hosts shouldn't know more about the card than the MMC core tells them.

Since I have no hardware for this, could you take it upon you to handle support for these chips? I'd like a MAINTAINERS patch for that as well.

Quoted 8 lines · Pierre Ossman

drivers/mmc/host/sdhci-s3c.c line 110

Quoted 3 lines · On Mon, 03 Nov 2008 20:09:49 +0000 Ben Dooks <[email protected]> wrote:

drivers/mmc/host/sdhci-s3c.c new file +3 -0

+	if (pdata->cfg_card)
+		pdata->cfg_card(ourhost->pdev, host->ioaddr,
+				ios, host->mmc->card);

What's the deal here? Hosts shouldn't know more about the card than the MMC core tells them.

Depending on exactly which version, there's a number of configurations in the extra configuration registers that get changed, such as delays used for clock feedback which are meant to be chnaged when the speed of the card is changed. Unfortunately this changes depending on the chip model and possibly also on the board it is on.

The other is that it is possible that depending on the chip and board combination things like the GPIO driver-strength controls will need to be changed.

Maybe I didn't understand it right, but if the DMA controller could overrun a buffer, don't you ALSO need to add defensive padding (i.e. increase the buffer) to make sure nothing important gets overrun?

Quoted 3 lines · Henrique de Moraes Holschuh

Maybe I didn't understand it right, but if the DMA controller could overrun a buffer, don't you ALSO need to add defensive padding (i.e. increase the buffer) to make sure nothing important gets overrun?

This is only generated by problems elsewhere in the driver, such as getting the timeout clock wrong. It is here just as a precaution and as an aide to debugging, it should not trigger in normal circumstances.

There is a seperate problem where the DMA buffer is passed from the stack which is, IIRC, a complete no-no under Linux.

Quoted 5 lines · Ben Dooks

Quoted 3 lines · Henrique de Moraes Holschuh

Maybe I didn't understand it right, but if the DMA controller could overrun a buffer, don't you ALSO need to add defensive padding (i.e. increase the buffer) to make sure nothing important gets overrun?

Quoted 3 lines

This is only generated by problems elsewhere in the driver, such as getting the timeout clock wrong. It is here just as a precaution and as an aide to debugging, it should not trigger in normal circumstances.

Then why is it just a WARN_ON, since you had a rogue DMA operation overwriting unknown kernel memory? Seems like an outright BUG_ON to me.

Quoted 2 lines

There is a seperate problem where the DMA buffer is passed from the stack which is, IIRC, a complete no-no under Linux.

Can't say much on that. I just found it strange that something as damaging as an overrun was only getting a WARN_ON and no defensive measure. If it is not going to happen normally, it might not require a defensive buffer, but once it happens, it looks like one must reboot ASAP from what you said...

At the end of a transfer, check that the DMA engine in the SDHCI controller actually did what it was meant to and didn't overrun the end of the buffer.

This seems to be triggered by a timeout during an CMD25 (multiple block write) to a card. The mmc_block module then issues a command to find out how much data was moved and this seems to end up triggering this DMA check. The result is the card's queue generates an OOPS as the stack has been trampled on due to the extra data transfered.

Signed-off-by Ben Dooks <[email protected]>

Quoted 10 lines · On Mon, 03 Nov 2008 20:09:50 +0000 Ben Dooks <[email protected]> wrote:

At the end of a transfer, check that the DMA engine in the SDHCI controller actually did what it was meant to and didn't overrun the end of the buffer.

This seems to be triggered by a timeout during an CMD25 (multiple block write) to a card. The mmc_block module then issues a command to find out how much data was moved and this seems to end up triggering this DMA check. The result is the card's queue generates an OOPS as the stack has been trampled on due to the extra data transfered.

Quoted 1 line

Signed-off-by Ben Dooks <[email protected]>

I'm sorry, but I don't see how this is anywhere near acceptable. This should be a panic at the very least, and until this can be sorted out and avoided the driver should avoid using DMA on these chips.

Quoted 4 lines · Pierre Ossman

I'm sorry, but I don't see how this is anywhere near acceptable. This should be a panic at the very least, and until this can be sorted out and avoided the driver should avoid using DMA on these chips.

A panic won't help get the debug logs out of the kernel.

This only
turned up whilst debugging the controller, I got the timeout clock calculation wrong and thus ended up timing out pretty much all the CMD25s and seeing this problem.

Add a change_clock callback to allow drivers to update device specific clock selections and control registers when there is a change in clock.

Move the main part of sdhci_set_clock() to a new routine which can be called by the glue drivers to do the sdhci standard clock management.

Update the sdhci-s3c driver to use this to select the appropriate clock source when clocks change.

Signed-off-by Ben Dooks <[email protected]>

Quoted 8 lines · On Mon, 03 Nov 2008 20:09:51 +0000 Ben Dooks <[email protected]> wrote:

Add a change_clock callback to allow drivers to update device specific clock selections and control registers when there is a change in clock.

Move the main part of sdhci_set_clock() to a new routine which can be called by the glue drivers to do the sdhci standard clock management.

Quoted 5 lines

Update the sdhci-s3c driver to use this to select the appropriate clock source when clocks change.

Signed-off-by Ben Dooks <[email protected]>

I'm afraid I don't quite follow on the requirements here. Care to elaborate as to why this is needed?

drivers/mmc/host/sdhci-pci.c static int sdhci_pci_enable_dma(struct s · line 397

Quoted 12 lines

Index: linux.git/drivers/mmc/host/sdhci-pci.c
===================================================================

drivers/mmc/host/sdhci-pci.c +1 -0

@@ -391,6 +391,7 @@ static int sdhci_pci_enable_dma(struct s
 
 static struct sdhci_ops sdhci_pci_ops = {
 	.enable_dma	= sdhci_pci_enable_dma,
+	.change_clock	= sdhci_change_clock,
 };
 
 /*****************************************************************************\

This seems pointless :)

drivers/mmc/host/sdhci-pci.c out: · line 959

Quoted 6 lines

drivers/mmc/host/sdhci-pci.c +2 -0

@@ -950,6 +955,8 @@ out:
 	host->clock = clock;
 }
 
+EXPORT_SYMBOL_GPL(sdhci_set_clock);
+

Wrong symbol. :)

The controller has a number of possible clock sources, the bus clock it is connected to (generally called hclk) and three more clocks from the heirachy that is muxed/divided from either an 48MHz, 27MHz or the EPLL.

The callback allows the recalculation and selection of the best clock to run the card clock for the controller.

The following patch set gets working SDHCI on the
Samsung SoC series.