sdhci: Best Practice Question

sdhci: Best Practice Question

[Philip Rakity]

and a 
host->ops-> get_max_clock()
function that is used to set the max clock.  The code snippet is below


	host->max_clk =
		(caps & SDHCI_CLOCK_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
	host->max_clk *= 1000000;
	if (host->max_clk == 0 || host->quirks &
			SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN) {
		if (!host->ops->get_max_clock) {
			printk(KERN_ERR
			       "%s: Hardware doesn't specify base clock "
			       "frequency.\n", mmc_hostname(mmc));
			return -ENODEV;
		}
		host->max_clk = host->ops->get_max_clock(host);
	}


The quirk seems redundant since this could be coded as follows to remove the need for the quirk

> +	if (host->ops->get_max_clock)
> 		host->max_clk = host->ops->get_max_clock(host);
> +	else {
> +		host->max_clk =
> +			(caps & SDHCI_CLOCK_BASE_MASK) 
> +				>> SDHCI_CLOCK_BASE_SHIFT;
> +		host->max_clk *= 1000000;
> +	}
> +
> +	if (host->max_clk == 0) {
> +		printk(KERN_ERR
> +		       "%s: Hardware doesn't specify base clock "
> +		       "frequency.\n", mmc_hostname(mmc));
> +		return -ENODEV;
> 	}
> 


This approach was not used since it was felt a quirk was needed.

http://kerneltrap.org/mailarchive/linux-kernel/2010/2/19/4540115

but the quirk redundant and it requires two items be set; the quirk and the callback.
Defining the quirk and not the callback is meaningless.  

The change proposed by Richard Zhu for handling write protect uses only a callback.

<snip>
static int sdhci_get_ro(struct mmc_host *mmc)
{
	struct sdhci_host *host;
	unsigned long flags;
	int present;

	host = mmc_priv(mmc);

	if (host->ops->get_ro)
		return host->ops->get_ro(host);

	spin_lock_irqsave(&host->lock, flags);

<end snip>

What is the correct practice?  

Philip

[PATCH] sdhci: sepearate get_ro into code that calls platform and helper

[Philip Rakity]

>From 0037e6513afc3fc39dd39010f994278c84623b29 Mon Sep 17 00:00:00 2001
From: Philip Rakity <prakity@marvell.com>
Date: Sat, 25 Sep 2010 17:23:59 -0700
Subject: [PATCH] sdhci: sepearate get_ro into code that calls platform and helper

If a helper function is supplied allow it to make use of the helper.
This allows specific behavior to be added when needed and reuse the standard
code
Signed-off-by: Philip Rakity <prakity@marvell.com>
---
 drivers/mmc/host/sdhci.c |   14 +++++++++++++-
 drivers/mmc/host/sdhci.h |    1 +
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0774dad..6f2bc4b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1204,7 +1204,7 @@ out:
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
-static int sdhci_get_ro(struct mmc_host *mmc)
+int sdhci_get_ro_helper(struct mmc_host *mmc)
 {
 	struct sdhci_host *host;
 	unsigned long flags;
@@ -1226,6 +1226,18 @@ static int sdhci_get_ro(struct mmc_host *mmc)
 	return !(present & SDHCI_WRITE_PROTECT);
 }
 
+int sdhci_get_ro(struct mmc_host *mmc)
+{
+	struct sdhci_host *host;
+
+	host = mmc_priv(mmc);
+
+	if (host->ops->get_ro)
+		return host->ops->get_ro(mmc);
+	else
+		return sdhci_get_ro_helper(mmc);
+}
+
 static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
 {
 	struct sdhci_host *host;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index f68af47..bd1fa15 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -324,6 +324,7 @@ struct sdhci_ops {
 	unsigned int	(*get_min_clock)(struct sdhci_host *host);
 	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
 	void	(*platform_specific_reset)(struct sdhci_host *host, u8 mask);
+	int	(*get_ro)(struct mmc_host *mmc);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 
1.6.0.4

[PATCH] sdhci: seperate out reset so if platform specific helper exists it is called

[Philip Rakity]

>From ae9c828941b7d170881ef62190b6d5d90d122ece Mon Sep 17 00:00:00 2001
From: Philip Rakity <prakity@marvell.com>
Date: Sat, 25 Sep 2010 17:36:42 -0700
Subject: [PATCH] sdhci: seperate out reset so if platform specific helper exists it is called
 if it exists otherwise standard reset function is called.  This way the platform
 specific code and use the standard code if needed.

for example in our driver we need to reset some platform specific registers depending
on the type of reset

a) if not reset_all -> just call the helper
b) if reset_all --> call the helper then do our private stuff

Signed-off-by: Philip Rakity <prakity@marvell.com>
---
 drivers/mmc/host/sdhci.c |   10 +++++++++-
 drivers/mmc/host/sdhci.h |    1 +
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 17f1554..451fad3 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -138,7 +138,7 @@ static void sdhci_disable_card_detection(struct sdhci_host *host)
 	sdhci_set_card_detection(host, false);
 }
 
-static void sdhci_reset(struct sdhci_host *host, u8 mask)
+void sdhci_reset_helper(struct sdhci_host *host, u8 mask)
 {
 	unsigned long timeout;
 	u32 uninitialized_var(ier);
@@ -176,6 +176,14 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
 		sdhci_clear_set_irqs(host, SDHCI_INT_ALL_MASK, ier);
 }
 
+static void sdhci_reset(struct sdhci_host *host, u8 mask)
+{
+	if (host->ops->sdhci_reset)
+		return host->ops->sdhci_reset(host, mask);
+	else
+		return sdhci_reset_helper(host, mask);
+}
+
 static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios);
 
 static void sdhci_init(struct sdhci_host *host, int soft)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index a41ad8c..f8c18cd 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -323,6 +323,7 @@ struct sdhci_ops {
 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
 	unsigned int	(*get_min_clock)(struct sdhci_host *host);
 	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
+	void	(*sdhci_reset)(struct sdhci_host *host, u8 mask);
 	int	(*get_ro)(struct mmc_host *mmc);
 };
 
-- 
1.6.0.4

Re: sdhci: Best Practice Question

[Chris Ball]

Hi Philip,

On Sat, Sep 25, 2010 at 01:08:20PM -0700, Philip Rakity wrote:
> The change proposed by Richard Zhu for handling write protect uses 
> only a callback.
> 
> <snip>
> static int sdhci_get_ro(struct mmc_host *mmc)
> {
> 	struct sdhci_host *host;
> 	unsigned long flags;
> 	int present;
> 
> 	host = mmc_priv(mmc);
> 
> 	if (host->ops->get_ro)
> 		return host->ops->get_ro(host);
> 
> 	spin_lock_irqsave(&host->lock, flags);
> 
> <end snip>
> 
> What is the correct practice?  

I think that the get_ro hook is reasonable in this case -- we're saying
that the host has a sufficiently weird WP setup that sdhci doesn't know 
what we're supposed to do (unlike SDHCI_QUIRK_INVERTED_WRITE_PROTECT).

I'd be curious to hear what others think, though.  Should we be simply
moving away from adding new quirks, or just limiting them to cases where
a full hook isn't warranted?

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[RFC] sdhci: SDHCI_QUIRK_BROKEN_CARD_DETECTION

[Philip Rakity]

The quirk SDHCI_QUIRK_BROKEN_CARD_DETECTION is not quite what may needed for embedded systems.

Sometimes the card is hard wired onto the board and is always present -- usually the signal into the controller
is not used by the board designed -- saves a pin

Sometimes the card is plugged into a slot and Card Detect REALLY is broken.  In this case we need to poll by 
setting MMC_CAP_NEEDS_POLL so mmc_detect_change() will find the card.

	if (host->caps & MMC_CAP_NEEDS_POLL)
		mmc_schedule_delayed_work(&host->detect, HZ);


Since the behavior is a board issue I think it makes more sense to define a new 
struct sdhci_ops {
}

that gets called and the adaption layer can decide to set or not set MMC_CAP_NEEDS_POLL.  if this is okay will
generate the patch.

Sample code snippets below

struct sdhci_ops {
	......
	void	(*card_detect)(struct sdhci_host *host);
}

====
replace 

	if (caps & SDHCI_CAN_DO_HISPD)
		mmc->caps |= MMC_CAP_SD_HIGHSPEED;

	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
		mmc->caps |= MMC_CAP_NEEDS_POLL;


with

	if (caps & SDHCI_CAN_DO_HISPD)
		mmc->caps |= MMC_CAP_SD_HIGHSPEED;

	if (host->ops->card_detect)
		host->ops->card_detect(host)
====

platform code 
void card_detect (struct sdhci_host *host)
{
	if (card_hard_wired || sd_slot_has_no_card_detect_signal) {
		host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
		if (sd_slot_has_no_card_detect_signal)
			host->mmc->caps |= MMC_CAP_NEEDS_POLL
	}
}