diff mbox

[3/6] mmc: sdhci: add platform set_timeout hook

Message ID 1386680168-5227-4-git-send-email-b29396@freescale.com (mailing list archive)
State New, archived
Headers show

Commit Message

Aisheng Dong Dec. 10, 2013, 12:56 p.m. UTC
Currently the common code assume 0xE is the maximum timeout counter
value and use it to write into the timeout counter register.
However, it's fairly possible that the different SoCs may have
different register layout on the timeout counter register.
That means 0xE may not be the correct maximum timeout value, then
the 0xE becomes meaningless.

To be flexible, this patch provides another approach for platforms
to set the correct timeout on their way.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/mmc/host/sdhci.c |   19 ++++++++++++++-----
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 16 insertions(+), 5 deletions(-)

Comments

Shawn Guo Dec. 11, 2013, 2:16 a.m. UTC | #1
On Tue, Dec 10, 2013 at 08:56:05PM +0800, Dong Aisheng wrote:
> Currently the common code assume 0xE is the maximum timeout counter
> value and use it to write into the timeout counter register.
> However, it's fairly possible that the different SoCs may have
> different register layout on the timeout counter register.
> That means 0xE may not be the correct maximum timeout value, then
> the 0xE becomes meaningless.
> 
> To be flexible, this patch provides another approach for platforms
> to set the correct timeout on their way.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  drivers/mmc/host/sdhci.c |   19 ++++++++++++++-----
>  drivers/mmc/host/sdhci.h |    2 ++
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 464d42c..4cc3bd6 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -726,19 +726,28 @@ static void sdhci_set_transfer_irqs(struct sdhci_host *host)
>  		sdhci_clear_set_irqs(host, dma_irqs, pio_irqs);
>  }
>  
> -static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
> +static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>  {
>  	u8 count;
> +
> +	if (host->ops->set_timeout) {
> +		host->ops->set_timeout(host, cmd);
> +	} else {
> +		count = sdhci_calc_timeout(host, cmd);
> +		sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
> +	}
> +}
> +
> +static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
> +{
>  	u8 ctrl;
>  	struct mmc_data *data = cmd->data;
>  	int ret;
>  
>  	WARN_ON(host->data);
>  
> -	if (data || (cmd->flags & MMC_RSP_BUSY)) {
> -		count = sdhci_calc_timeout(host, cmd);

From what I read the commit log, I think it might be more appropriate to
patch sdhci_calc_timeout() like the following?

	if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
		if (host->ops->get_max_timeout)
			return host->ops->get_max_timeout(host);
		else
			return 0xE;

Shawn

> -		sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
> -	}
> +	if (data || (cmd->flags & MMC_RSP_BUSY))
> +		sdhci_set_timeout(host, cmd);
>  
>  	if (!data)
>  		return;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 1591cbb..a4851a0 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -282,6 +282,8 @@ struct sdhci_ops {
>  	unsigned int	(*get_min_clock)(struct sdhci_host *host);
>  	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
>  	unsigned int	(*get_max_timeout)(struct sdhci_host *host);
> +	void		(*set_timeout)(struct sdhci_host *host,
> +						struct mmc_command *cmd);
>  	int		(*platform_bus_width)(struct sdhci_host *host,
>  					       int width);
>  	void (*platform_send_init_74_clocks)(struct sdhci_host *host,
> -- 
> 1.7.2.rc3
> 
>
Dong Aisheng Dec. 11, 2013, 3:03 a.m. UTC | #2
On Wed, Dec 11, 2013 at 10:16 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, Dec 10, 2013 at 08:56:05PM +0800, Dong Aisheng wrote:
>> Currently the common code assume 0xE is the maximum timeout counter
>> value and use it to write into the timeout counter register.
>> However, it's fairly possible that the different SoCs may have
>> different register layout on the timeout counter register.
>> That means 0xE may not be the correct maximum timeout value, then
>> the 0xE becomes meaningless.
>>
>> To be flexible, this patch provides another approach for platforms
>> to set the correct timeout on their way.
>>
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>> ---
>>  drivers/mmc/host/sdhci.c |   19 ++++++++++++++-----
>>  drivers/mmc/host/sdhci.h |    2 ++
>>  2 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 464d42c..4cc3bd6 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -726,19 +726,28 @@ static void sdhci_set_transfer_irqs(struct sdhci_host *host)
>>               sdhci_clear_set_irqs(host, dma_irqs, pio_irqs);
>>  }
>>
>> -static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>> +static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>>  {
>>       u8 count;
>> +
>> +     if (host->ops->set_timeout) {
>> +             host->ops->set_timeout(host, cmd);
>> +     } else {
>> +             count = sdhci_calc_timeout(host, cmd);
>> +             sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
>> +     }
>> +}
>> +
>> +static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>> +{
>>       u8 ctrl;
>>       struct mmc_data *data = cmd->data;
>>       int ret;
>>
>>       WARN_ON(host->data);
>>
>> -     if (data || (cmd->flags & MMC_RSP_BUSY)) {
>> -             count = sdhci_calc_timeout(host, cmd);
>
> From what I read the commit log, I think it might be more appropriate to
> patch sdhci_calc_timeout() like the following?
>
>         if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>                 if (host->ops->get_max_timeout)
>                         return host->ops->get_max_timeout(host);
>                 else
>                         return 0xE;
>

The return val of sdhci_calc_timeout is the register value to be
written into timeout counter register.
host->ops->get_max_timeout returns the max timeout in miliseconds directly.
So we can not do like that here.
They're two concepts.

Regards
Dong Aisheng

> Shawn
>
>> -             sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
>> -     }
>> +     if (data || (cmd->flags & MMC_RSP_BUSY))
>> +             sdhci_set_timeout(host, cmd);
>>
>>       if (!data)
>>               return;
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 1591cbb..a4851a0 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -282,6 +282,8 @@ struct sdhci_ops {
>>       unsigned int    (*get_min_clock)(struct sdhci_host *host);
>>       unsigned int    (*get_timeout_clock)(struct sdhci_host *host);
>>       unsigned int    (*get_max_timeout)(struct sdhci_host *host);
>> +     void            (*set_timeout)(struct sdhci_host *host,
>> +                                             struct mmc_command *cmd);
>>       int             (*platform_bus_width)(struct sdhci_host *host,
>>                                              int width);
>>       void (*platform_send_init_74_clocks)(struct sdhci_host *host,
>> --
>> 1.7.2.rc3
>>
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Shawn Guo Dec. 11, 2013, 3:18 a.m. UTC | #3
On Wed, Dec 11, 2013 at 11:03:31AM +0800, Dong Aisheng wrote:
> >> +static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
> >> +{
> >>       u8 ctrl;
> >>       struct mmc_data *data = cmd->data;
> >>       int ret;
> >>
> >>       WARN_ON(host->data);
> >>
> >> -     if (data || (cmd->flags & MMC_RSP_BUSY)) {
> >> -             count = sdhci_calc_timeout(host, cmd);
> >
> > From what I read the commit log, I think it might be more appropriate to
> > patch sdhci_calc_timeout() like the following?
> >
> >         if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
> >                 if (host->ops->get_max_timeout)
> >                         return host->ops->get_max_timeout(host);
> >                 else
> >                         return 0xE;
> >
> 
> The return val of sdhci_calc_timeout is the register value to be
> written into timeout counter register.
> host->ops->get_max_timeout returns the max timeout in miliseconds directly.
> So we can not do like that here.
> They're two concepts.

I did not make my comment clear.  The .get_max_timeout() is not the one
you defined in patch #1 any more, but something like the following for
esdhc driver, which returns correct timeout value not milliseconds.

unsigned int esdhc_get_max_timeout(struct sdhci_host *host)
{
	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
	struct pltfm_imx_data *imx_data = pltfm_host->priv;

	return esdhc_is_usdhc(imx_data) ? 0xF : 0xE;

}

Does that match the problem you described in the commit log better?

Shawn
Dong Aisheng Dec. 11, 2013, 3:35 a.m. UTC | #4
On Wed, Dec 11, 2013 at 11:18 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Wed, Dec 11, 2013 at 11:03:31AM +0800, Dong Aisheng wrote:
>> >> +static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>> >> +{
>> >>       u8 ctrl;
>> >>       struct mmc_data *data = cmd->data;
>> >>       int ret;
>> >>
>> >>       WARN_ON(host->data);
>> >>
>> >> -     if (data || (cmd->flags & MMC_RSP_BUSY)) {
>> >> -             count = sdhci_calc_timeout(host, cmd);
>> >
>> > From what I read the commit log, I think it might be more appropriate to
>> > patch sdhci_calc_timeout() like the following?
>> >
>> >         if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>> >                 if (host->ops->get_max_timeout)
>> >                         return host->ops->get_max_timeout(host);
>> >                 else
>> >                         return 0xE;
>> >
>>
>> The return val of sdhci_calc_timeout is the register value to be
>> written into timeout counter register.
>> host->ops->get_max_timeout returns the max timeout in miliseconds directly.
>> So we can not do like that here.
>> They're two concepts.
>
> I did not make my comment clear.  The .get_max_timeout() is not the one
> you defined in patch #1 any more, but something like the following for
> esdhc driver, which returns correct timeout value not milliseconds.
>
> unsigned int esdhc_get_max_timeout(struct sdhci_host *host)
> {
>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>         struct pltfm_imx_data *imx_data = pltfm_host->priv;
>
>         return esdhc_is_usdhc(imx_data) ? 0xF : 0xE;
>
> }
>
> Does that match the problem you described in the commit log better?
>

This actually is the register value, not the max timeout counter value.
Then you may want define the API as:
unsigned int    (*get_max_timeout_val)(struct sdhci_host *host);
But i don't think it's necessary to do the max timeout setting in two steps.
First, deinfe a API to get the max timeout counter val,
Second, write this val into register.
Why not simply implement .set_timeout and handle the details in
platform specific
host driver respectively?

Furthermore, this API does not  help for the patch#1 issue.


Regards
Dong Aisheng


> Shawn
>
Shawn Guo Dec. 11, 2013, 3:48 a.m. UTC | #5
On Wed, Dec 11, 2013 at 11:35:17AM +0800, Dong Aisheng wrote:
> This actually is the register value, not the max timeout counter value.
> Then you may want define the API as:
> unsigned int    (*get_max_timeout_val)(struct sdhci_host *host);

Yes, something like that.

> But i don't think it's necessary to do the max timeout setting in two steps.
> First, deinfe a API to get the max timeout counter val,
> Second, write this val into register.
> Why not simply implement .set_timeout and handle the details in
> platform specific
> host driver respectively?

Well, that's how sdhci host driver is structured.  Doing so leaves the
least details to platform driver, and calling sdhci_writeb() to access
SDHCI_TIMEOUT_CONTROL in sdhci-esdhc-imx seems a layer violation to me.

> 
> Furthermore, this API does not  help for the patch#1 issue.

Oh, they two different issues, and should be addressed by different
hooks.

Shawn
Dong Aisheng Dec. 11, 2013, 5:03 a.m. UTC | #6
On Wed, Dec 11, 2013 at 11:48 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Wed, Dec 11, 2013 at 11:35:17AM +0800, Dong Aisheng wrote:
>> This actually is the register value, not the max timeout counter value.
>> Then you may want define the API as:
>> unsigned int    (*get_max_timeout_val)(struct sdhci_host *host);
>
> Yes, something like that.
>
>> But i don't think it's necessary to do the max timeout setting in two steps.
>> First, deinfe a API to get the max timeout counter val,
>> Second, write this val into register.
>> Why not simply implement .set_timeout and handle the details in
>> platform specific
>> host driver respectively?
>
> Well, that's how sdhci host driver is structured.  Doing so leaves the
> least details to platform driver, and calling sdhci_writeb() to access
> SDHCI_TIMEOUT_CONTROL in sdhci-esdhc-imx seems a layer violation to me.
>

The current sdhci-esdhci-imx already does something like that.
You can search SDHCI_* in the code.
It just reuses the register offset definition in sdhci,
It's not the layer violation.

Regards
Dong Aisheng

>>
>> Furthermore, this API does not  help for the patch#1 issue.
>
> Oh, they two different issues, and should be addressed by different
> hooks.
>
> Shawn
>
Shawn Guo Dec. 11, 2013, 6:04 a.m. UTC | #7
On Wed, Dec 11, 2013 at 01:03:43PM +0800, Dong Aisheng wrote:
> On Wed, Dec 11, 2013 at 11:48 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > On Wed, Dec 11, 2013 at 11:35:17AM +0800, Dong Aisheng wrote:
> >> This actually is the register value, not the max timeout counter value.
> >> Then you may want define the API as:
> >> unsigned int    (*get_max_timeout_val)(struct sdhci_host *host);
> >
> > Yes, something like that.
> >
> >> But i don't think it's necessary to do the max timeout setting in two steps.
> >> First, deinfe a API to get the max timeout counter val,
> >> Second, write this val into register.
> >> Why not simply implement .set_timeout and handle the details in
> >> platform specific
> >> host driver respectively?
> >
> > Well, that's how sdhci host driver is structured.  Doing so leaves the
> > least details to platform driver, and calling sdhci_writeb() to access
> > SDHCI_TIMEOUT_CONTROL in sdhci-esdhc-imx seems a layer violation to me.
> >
> 
> The current sdhci-esdhci-imx already does something like that.
> You can search SDHCI_* in the code.
> It just reuses the register offset definition in sdhci,
> It's not the layer violation.

I do not understand why we have to do this SDHCI_TIMEOUT_CONTROL setup
in our sdhci-esdhc-imx driver, when sdhci driver is already structured
to do it in its way.  All we need is a hook to give the correct
max_timeout_val.

Shawn
Dong Aisheng Dec. 17, 2013, 6:49 a.m. UTC | #8
On Wed, Dec 11, 2013 at 2:04 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Wed, Dec 11, 2013 at 01:03:43PM +0800, Dong Aisheng wrote:
>> On Wed, Dec 11, 2013 at 11:48 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>> > On Wed, Dec 11, 2013 at 11:35:17AM +0800, Dong Aisheng wrote:
>> >> This actually is the register value, not the max timeout counter value.
>> >> Then you may want define the API as:
>> >> unsigned int    (*get_max_timeout_val)(struct sdhci_host *host);
>> >
>> > Yes, something like that.
>> >
>> >> But i don't think it's necessary to do the max timeout setting in two steps.
>> >> First, deinfe a API to get the max timeout counter val,
>> >> Second, write this val into register.
>> >> Why not simply implement .set_timeout and handle the details in
>> >> platform specific
>> >> host driver respectively?
>> >
>> > Well, that's how sdhci host driver is structured.  Doing so leaves the
>> > least details to platform driver, and calling sdhci_writeb() to access
>> > SDHCI_TIMEOUT_CONTROL in sdhci-esdhc-imx seems a layer violation to me.
>> >
>>
>> The current sdhci-esdhci-imx already does something like that.
>> You can search SDHCI_* in the code.
>> It just reuses the register offset definition in sdhci,
>> It's not the layer violation.
>
> I do not understand why we have to do this SDHCI_TIMEOUT_CONTROL setup
> in our sdhci-esdhc-imx driver, when sdhci driver is already structured
> to do it in its way.  All we need is a hook to give the correct
> max_timeout_val.
>

Because i don't think sdhci_calc_timeout is still meaningful anymore for a
SDHCI_QUIRK_BROKEN_TIMEOUT_VAL anymore.
The only thing you reply sdhci to do is set a max_timeout.

And i don't think it's necessary to use a hook to get max_timeout_val first,
then use this val to write into register later to do the max_timeout
setting work.
It's perfect fine to let the platform .set_timeout to do it directly.
And we already have .get_max_timeout_count hook, i don't want
.get_max_timeout_val again.

There's another important reason that .set_timeout provides other platforms
host controller ability to set timeout based on their way if they want.
The sdhci should have this capability since it is common case just like
.set_clock and etc.
e.g. imx5. the timeout count setting is different when DDR is enabled.
(Though we still not add DDR support for mx5)
Then we must need such hook to do specific IMX timeout setting instead
of using common sdhci timeout setting.

Regards
Dong Aisheng

> Shawn
>
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 464d42c..4cc3bd6 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -726,19 +726,28 @@  static void sdhci_set_transfer_irqs(struct sdhci_host *host)
 		sdhci_clear_set_irqs(host, dma_irqs, pio_irqs);
 }
 
-static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
+static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 count;
+
+	if (host->ops->set_timeout) {
+		host->ops->set_timeout(host, cmd);
+	} else {
+		count = sdhci_calc_timeout(host, cmd);
+		sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
+	}
+}
+
+static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
+{
 	u8 ctrl;
 	struct mmc_data *data = cmd->data;
 	int ret;
 
 	WARN_ON(host->data);
 
-	if (data || (cmd->flags & MMC_RSP_BUSY)) {
-		count = sdhci_calc_timeout(host, cmd);
-		sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
-	}
+	if (data || (cmd->flags & MMC_RSP_BUSY))
+		sdhci_set_timeout(host, cmd);
 
 	if (!data)
 		return;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 1591cbb..a4851a0 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -282,6 +282,8 @@  struct sdhci_ops {
 	unsigned int	(*get_min_clock)(struct sdhci_host *host);
 	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
 	unsigned int	(*get_max_timeout)(struct sdhci_host *host);
+	void		(*set_timeout)(struct sdhci_host *host,
+						struct mmc_command *cmd);
 	int		(*platform_bus_width)(struct sdhci_host *host,
 					       int width);
 	void (*platform_send_init_74_clocks)(struct sdhci_host *host,