Patchwork [3/3] mmc: Add ESDHC weird register workaround

login
register
mail settings
Submitter Zang Roy-R61911
Date July 28, 2010, 5:54 a.m.
Message ID <1280296461-17077-1-git-send-email-tie-fei.zang@freescale.com>
Download mbox | patch
Permalink /patch/60106/
State Not Applicable
Headers show

Comments

Zang Roy-R61911 - July 28, 2010, 5:54 a.m.
P4080 ESDHC controller induces weird register setting.
This patch adds the workaround to correct the weird register setting.

Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
---
 drivers/mmc/host/sdhci-of-core.c |    5 +++++
 drivers/mmc/host/sdhci.c         |   13 +++++++++++++
 drivers/mmc/host/sdhci.h         |    2 ++
 3 files changed, 20 insertions(+), 0 deletions(-)
Anton Vorontsov - July 28, 2010, 8:03 a.m.
On Wed, Jul 28, 2010 at 01:54:21PM +0800, Roy Zang wrote:
> P4080 ESDHC controller induces weird register setting.
> This patch adds the workaround to correct the weird register setting.
> 
> Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
> ---
>  drivers/mmc/host/sdhci-of-core.c |    5 +++++
>  drivers/mmc/host/sdhci.c         |   13 +++++++++++++
>  drivers/mmc/host/sdhci.h         |    2 ++
>  3 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-core.c b/drivers/mmc/host/sdhci-of-core.c
> index 0c30242..1b6945a 100644
> --- a/drivers/mmc/host/sdhci-of-core.c
> +++ b/drivers/mmc/host/sdhci-of-core.c
> @@ -164,6 +164,11 @@ static int __devinit sdhci_of_probe(struct of_device *ofdev,
>  	if (sdhci_of_wp_inverted(np))
>  		host->quirks |= SDHCI_QUIRK_INVERTED_WRITE_PROTECT;
>  
> +	if (of_device_is_compatible(np, "fsl,p4080-esdhc")) {
> +		host->quirks |= SDHCI_QUIRK_QORIQ_REG_WEIRD;
> +		host->quirks &= ~SDHCI_QUIRK_INVERTED_WRITE_PROTECT;

You do not mention this change in the patch description, why?

> +	}
> +
>  	clk = of_get_property(np, "clock-frequency", &size);
>  	if (clk && size == sizeof(*clk) && *clk)
>  		of_host->clock = *clk;
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1424d08..b5b3627 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -788,6 +788,15 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
>  		sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>  	}
>  
> +	/* The default value of DMAS bits of Protocol Control Register is not
> +	 * correct. clear these two bits to use simple DMA */
> +#define  ESDHCI_CTRL_DMAS_MASK		0xFFFFFCFF
> +	if (host->quirks & SDHCI_QUIRK_QORIQ_REG_WEIRD) {
> +		ctrl = sdhci_readl(host, SDHCI_HOST_CONTROL);
> +		ctrl = ctrl & ESDHCI_CTRL_DMAS_MASK;
> +		sdhci_writel(host, ctrl, SDHCI_HOST_CONTROL);
> +	}

You should implement register quirks via SDHCI IO accessors.

See esdhc_writew() for example.

>  	if (!(host->flags & SDHCI_REQ_USE_DMA)) {
>  		int flags;
>  
> @@ -1699,6 +1708,10 @@ int sdhci_add_host(struct sdhci_host *host)
>  
>  	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
>  
> +	 /* Workaround for P4080 host controller capabilities */
> +	if (host->quirks & SDHCI_QUIRK_QORIQ_REG_WEIRD)
> +		caps &= ~(SDHCI_CAN_VDD_180 | SDHCI_CAN_VDD_330);

Ditto. Make a quirk for cap register. Or implement
SDHCI_QUIRK_NO_VDD_180 and SDHCI_QUIRK_NO_VDD_300 quirks
instead.

Thanks,
Zang Roy-R61911 - July 28, 2010, 9:43 a.m.
> -----Original Message-----
> From: Anton Vorontsov [mailto:cbouatmailru@gmail.com] 
> Sent: Wednesday, July 28, 2010 16:03 PM
> To: Zang Roy-R61911
> Cc: linux-mmc-approval@vger.kernel.org; linuxppc-dev@ozlabs.org
> Subject: Re: [PATCH 3/3] mmc: Add ESDHC weird register workaround
> 
> On Wed, Jul 28, 2010 at 01:54:21PM +0800, Roy Zang wrote:
> > P4080 ESDHC controller induces weird register setting.
> > This patch adds the workaround to correct the weird 
> register setting.
> > 
> > Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
> > ---
> >  drivers/mmc/host/sdhci-of-core.c |    5 +++++
> >  drivers/mmc/host/sdhci.c         |   13 +++++++++++++
> >  drivers/mmc/host/sdhci.h         |    2 ++
> >  3 files changed, 20 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci-of-core.c 
> b/drivers/mmc/host/sdhci-of-core.c
> > index 0c30242..1b6945a 100644
> > --- a/drivers/mmc/host/sdhci-of-core.c
> > +++ b/drivers/mmc/host/sdhci-of-core.c
> > @@ -164,6 +164,11 @@ static int __devinit 
> sdhci_of_probe(struct of_device *ofdev,
> >  	if (sdhci_of_wp_inverted(np))
> >  		host->quirks |= SDHCI_QUIRK_INVERTED_WRITE_PROTECT;
> >  
> > +	if (of_device_is_compatible(np, "fsl,p4080-esdhc")) {
> > +		host->quirks |= SDHCI_QUIRK_QORIQ_REG_WEIRD;
> > +		host->quirks &= ~SDHCI_QUIRK_INVERTED_WRITE_PROTECT;
> 
> You do not mention this change in the patch description, why?
This is not needed any more. Will be removed.
> 
> > +	}
> > +
> >  	clk = of_get_property(np, "clock-frequency", &size);
> >  	if (clk && size == sizeof(*clk) && *clk)
> >  		of_host->clock = *clk;
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 1424d08..b5b3627 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -788,6 +788,15 @@ static void sdhci_prepare_data(struct 
> sdhci_host *host, struct mmc_data *data)
> >  		sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> >  	}
> >  
> > +	/* The default value of DMAS bits of Protocol Control 
> Register is not
> > +	 * correct. clear these two bits to use simple DMA */
> > +#define  ESDHCI_CTRL_DMAS_MASK		0xFFFFFCFF
> > +	if (host->quirks & SDHCI_QUIRK_QORIQ_REG_WEIRD) {
> > +		ctrl = sdhci_readl(host, SDHCI_HOST_CONTROL);
> > +		ctrl = ctrl & ESDHCI_CTRL_DMAS_MASK;
> > +		sdhci_writel(host, ctrl, SDHCI_HOST_CONTROL);
> > +	}
> 
> You should implement register quirks via SDHCI IO accessors.
What does you mean "SDHCI IO accessors?
It makes more sense to define something like:
SDHCI_QUIRK_QORIQ_PROCTL_WEIRD.
Any comment?

> 
> See esdhc_writew() for example.
> 
> >  	if (!(host->flags & SDHCI_REQ_USE_DMA)) {
> >  		int flags;
> >  
> > @@ -1699,6 +1708,10 @@ int sdhci_add_host(struct sdhci_host *host)
> >  
> >  	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> >  
> > +	 /* Workaround for P4080 host controller capabilities */
> > +	if (host->quirks & SDHCI_QUIRK_QORIQ_REG_WEIRD)
> > +		caps &= ~(SDHCI_CAN_VDD_180 | SDHCI_CAN_VDD_330);
> 
> Ditto. Make a quirk for cap register. Or implement
> SDHCI_QUIRK_NO_VDD_180 and SDHCI_QUIRK_NO_VDD_300 quirks
> instead.
Second proposal more makes sense to me.
Thanks.
Roy
Anton Vorontsov - July 30, 2010, 7:37 a.m.
On Wed, Jul 28, 2010 at 05:43:00PM +0800, Zang Roy-R61911 wrote:
[...]
> > > +	/* The default value of DMAS bits of Protocol Control 
> > Register is not
> > > +	 * correct. clear these two bits to use simple DMA */
> > > +#define  ESDHCI_CTRL_DMAS_MASK		0xFFFFFCFF
> > > +	if (host->quirks & SDHCI_QUIRK_QORIQ_REG_WEIRD) {
> > > +		ctrl = sdhci_readl(host, SDHCI_HOST_CONTROL);
> > > +		ctrl = ctrl & ESDHCI_CTRL_DMAS_MASK;
> > > +		sdhci_writel(host, ctrl, SDHCI_HOST_CONTROL);
> > > +	}
> > 
> > You should implement register quirks via SDHCI IO accessors.
> What does you mean "SDHCI IO accessors?

See drivers/mmc/host/sdhci-of-esdhc.c

struct sdhci_of_data sdhci_esdhc = {
...
        .ops = {
...
                .write_l = sdhci_be32bs_writel,
...
        },
};

You can implement your own write_l and put the quirk in
there, as we do for esdhc_readw, esdhc_writew, etc.

OTOH, as you say it has wrong default value, not behaviour,
so I think you can just initialize the HOST_CONTROL register
just once somewhere in sdhci-of-esdhc, e.g. ->enable_dma
callback?

> It makes more sense to define something like:
> SDHCI_QUIRK_QORIQ_PROCTL_WEIRD.

I don't think so.

Thanks,

Patch

diff --git a/drivers/mmc/host/sdhci-of-core.c b/drivers/mmc/host/sdhci-of-core.c
index 0c30242..1b6945a 100644
--- a/drivers/mmc/host/sdhci-of-core.c
+++ b/drivers/mmc/host/sdhci-of-core.c
@@ -164,6 +164,11 @@  static int __devinit sdhci_of_probe(struct of_device *ofdev,
 	if (sdhci_of_wp_inverted(np))
 		host->quirks |= SDHCI_QUIRK_INVERTED_WRITE_PROTECT;
 
+	if (of_device_is_compatible(np, "fsl,p4080-esdhc")) {
+		host->quirks |= SDHCI_QUIRK_QORIQ_REG_WEIRD;
+		host->quirks &= ~SDHCI_QUIRK_INVERTED_WRITE_PROTECT;
+	}
+
 	clk = of_get_property(np, "clock-frequency", &size);
 	if (clk && size == sizeof(*clk) && *clk)
 		of_host->clock = *clk;
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1424d08..b5b3627 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -788,6 +788,15 @@  static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 		sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
 	}
 
+	/* The default value of DMAS bits of Protocol Control Register is not
+	 * correct. clear these two bits to use simple DMA */
+#define  ESDHCI_CTRL_DMAS_MASK		0xFFFFFCFF
+	if (host->quirks & SDHCI_QUIRK_QORIQ_REG_WEIRD) {
+		ctrl = sdhci_readl(host, SDHCI_HOST_CONTROL);
+		ctrl = ctrl & ESDHCI_CTRL_DMAS_MASK;
+		sdhci_writel(host, ctrl, SDHCI_HOST_CONTROL);
+	}
+
 	if (!(host->flags & SDHCI_REQ_USE_DMA)) {
 		int flags;
 
@@ -1699,6 +1708,10 @@  int sdhci_add_host(struct sdhci_host *host)
 
 	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
 
+	 /* Workaround for P4080 host controller capabilities */
+	if (host->quirks & SDHCI_QUIRK_QORIQ_REG_WEIRD)
+		caps &= ~(SDHCI_CAN_VDD_180 | SDHCI_CAN_VDD_330);
+
 	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
 		host->flags |= SDHCI_USE_SDMA;
 	else if (!(caps & SDHCI_CAN_DO_SDMA))
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index aa112aa..33d5613 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -243,6 +243,8 @@  struct sdhci_host {
 #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
 /* Controller uses Auto CMD12 command to stop the transfer */
 #define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12		(1<<27)
+/* Controller has weird bit setting for some registers due to errata */
+#define SDHCI_QUIRK_QORIQ_REG_WEIRD			(1<<28)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */