diff mbox

[1/2] mmc: sdhci-s3c: add default controller configuration

Message ID 1314976702-19284-2-git-send-email-thomas.abraham@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Abraham Sept. 2, 2011, 3:18 p.m. UTC
The default controller configuration which was previously setup by
platform helper functions is moved into the driver.

Cc: Ben Dooks <ben-linux@fluff.org>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 drivers/mmc/host/sdhci-s3c.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

Comments

Jaehoon Chung Sept. 5, 2011, 1:31 a.m. UTC | #1
Hi Thomas.

I have some question. This patch looks good.
Some names are used S3C_SDHCI_xxxx. but some names are used S3C64XX_SDHCI_xxxx.
Do you know what differ them?

Thanks,
Jaehoon Chung

Thomas Abraham wrote:

> The default controller configuration which was previously setup by
> platform helper functions is moved into the driver.
> 
> Cc: Ben Dooks <ben-linux@fluff.org>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  drivers/mmc/host/sdhci-s3c.c |   28 +++++++++++++++++-----------
>  1 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 2bd7bf4..d891682 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -203,17 +203,23 @@ static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock)
>  		writel(ctrl, host->ioaddr + S3C_SDHCI_CONTROL2);
>  	}
>  
> -	/* reconfigure the hardware for new clock rate */
> -
> -	{
> -		struct mmc_ios ios;
> -
> -		ios.clock = clock;
> -
> -		if (ourhost->pdata->cfg_card)
> -			(ourhost->pdata->cfg_card)(ourhost->pdev, host->ioaddr,
> -						   &ios, NULL);
> -	}
> +	/* reprogram default hardware configuration */
> +	writel(S3C64XX_SDHCI_CONTROL4_DRIVE_9mA,
> +		host->ioaddr + S3C64XX_SDHCI_CONTROL4);
> +
> +	ctrl = readl(host->ioaddr + S3C_SDHCI_CONTROL2);
> +	ctrl |= (S3C64XX_SDHCI_CTRL2_ENSTAASYNCCLR |
> +		  S3C64XX_SDHCI_CTRL2_ENCMDCNFMSK |
> +		  S3C_SDHCI_CTRL2_ENFBCLKRX |
> +		  S3C_SDHCI_CTRL2_DFCNT_NONE |
> +		  S3C_SDHCI_CTRL2_ENCLKOUTHOLD);
> +	writel(ctrl, host->ioaddr + S3C_SDHCI_CONTROL2);
> +
> +	/* reconfigure the controller for new clock rate */
> +	ctrl = (S3C_SDHCI_CTRL3_FCSEL1 | S3C_SDHCI_CTRL3_FCSEL0);
> +	if (clock < 25 * 1000000)
> +		ctrl |= (S3C_SDHCI_CTRL3_FCSEL3 | S3C_SDHCI_CTRL3_FCSEL2);
> +	writel(ctrl, host->ioaddr + S3C_SDHCI_CONTROL3);
>  }
>  
>  /**


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kim Kukjin Sept. 5, 2011, 5:16 a.m. UTC | #2
Thomas Abraham wrote:
> 
> The default controller configuration which was previously setup by
> platform helper functions is moved into the driver.
> 
> Cc: Ben Dooks <ben-linux@fluff.org>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  drivers/mmc/host/sdhci-s3c.c |   28 +++++++++++++++++-----------
>  1 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 2bd7bf4..d891682 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -203,17 +203,23 @@ static void sdhci_s3c_set_clock(struct sdhci_host
*host,
> unsigned int clock)
>  		writel(ctrl, host->ioaddr + S3C_SDHCI_CONTROL2);
>  	}
> 
> -	/* reconfigure the hardware for new clock rate */
> -
> -	{
> -		struct mmc_ios ios;
> -
> -		ios.clock = clock;
> -
> -		if (ourhost->pdata->cfg_card)
> -			(ourhost->pdata->cfg_card)(ourhost->pdev, host-
> >ioaddr,
> -						   &ios, NULL);
> -	}
> +	/* reprogram default hardware configuration */
> +	writel(S3C64XX_SDHCI_CONTROL4_DRIVE_9mA,
> +		host->ioaddr + S3C64XX_SDHCI_CONTROL4);

Since there are above codes on only S5PC100 and S5PV210, I'm not sure above
is needed on other Samsung SoCs.

I need to sort out checking.

> +
> +	ctrl = readl(host->ioaddr + S3C_SDHCI_CONTROL2);

+	ctrl &= S3C_SDHCI_CTRL2_SELBASECLK_MASK; ?

> +	ctrl |= (S3C64XX_SDHCI_CTRL2_ENSTAASYNCCLR |
> +		  S3C64XX_SDHCI_CTRL2_ENCMDCNFMSK |
> +		  S3C_SDHCI_CTRL2_ENFBCLKRX |
> +		  S3C_SDHCI_CTRL2_DFCNT_NONE |
> +		  S3C_SDHCI_CTRL2_ENCLKOUTHOLD);
> +	writel(ctrl, host->ioaddr + S3C_SDHCI_CONTROL2);
> +
> +	/* reconfigure the controller for new clock rate */
> +	ctrl = (S3C_SDHCI_CTRL3_FCSEL1 | S3C_SDHCI_CTRL3_FCSEL0);
> +	if (clock < 25 * 1000000)
> +		ctrl |= (S3C_SDHCI_CTRL3_FCSEL3 |
> S3C_SDHCI_CTRL3_FCSEL2);
> +	writel(ctrl, host->ioaddr + S3C_SDHCI_CONTROL3);
>  }
> 
>  /**
> --

Basically, it's good to move common codes and to remove that. But I'm not
sure we don't _really_ need to keep SoC specific control function such as
cfg_card().

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Abraham Sept. 5, 2011, 7:58 a.m. UTC | #3
Hi Jaehoon,

On 5 September 2011 07:01, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Hi Thomas.
>
> I have some question. This patch looks good.
> Some names are used S3C_SDHCI_xxxx. but some names are used S3C64XX_SDHCI_xxxx.
> Do you know what differ them?

I am not sure why there are S3C_SDHCI_xxx and S3C64XX_SDHCI_xxx
macros. This patch reused the existing macros from regs-sdhci.h file.
It should be possible to rename S3C64XX_SDHCI_xxx to S3C_SDHCI_xxx.

Thanks,
Thomas.

>
> Thanks,
> Jaehoon Chung
>
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Abraham Sept. 5, 2011, 8:29 a.m. UTC | #4
Dear Mr. Kim,

On 5 September 2011 10:46, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Thomas Abraham wrote:
>>
>> The default controller configuration which was previously setup by
>> platform helper functions is moved into the driver.
>>
>> Cc: Ben Dooks <ben-linux@fluff.org>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>>  drivers/mmc/host/sdhci-s3c.c |   28 +++++++++++++++++-----------
>>  1 files changed, 17 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
>> index 2bd7bf4..d891682 100644
>> --- a/drivers/mmc/host/sdhci-s3c.c
>> +++ b/drivers/mmc/host/sdhci-s3c.c
>> @@ -203,17 +203,23 @@ static void sdhci_s3c_set_clock(struct sdhci_host
> *host,
>> unsigned int clock)
>>               writel(ctrl, host->ioaddr + S3C_SDHCI_CONTROL2);
>>       }
>>
>> -     /* reconfigure the hardware for new clock rate */
>> -
>> -     {
>> -             struct mmc_ios ios;
>> -
>> -             ios.clock = clock;
>> -
>> -             if (ourhost->pdata->cfg_card)
>> -                     (ourhost->pdata->cfg_card)(ourhost->pdev, host-
>> >ioaddr,
>> -                                                &ios, NULL);
>> -     }
>> +     /* reprogram default hardware configuration */
>> +     writel(S3C64XX_SDHCI_CONTROL4_DRIVE_9mA,
>> +             host->ioaddr + S3C64XX_SDHCI_CONTROL4);
>
> Since there are above codes on only S5PC100 and S5PV210, I'm not sure above
> is needed on other Samsung SoCs.
>
> I need to sort out checking.


s3c6410, s5p6440, s5p6450 and s5pc110 SoC's include support for clock
out pad driver strength. All other SoC's do not use and list the two
bits as reserved. I do not know if there is any problem writing to
these reserved bits. But I have tested this patch on all the boards
that do not support this feature and it did not break anything.

>
>> +
>> +     ctrl = readl(host->ioaddr + S3C_SDHCI_CONTROL2);
>
> +       ctrl &= S3C_SDHCI_CTRL2_SELBASECLK_MASK; ?
>
>> +     ctrl |= (S3C64XX_SDHCI_CTRL2_ENSTAASYNCCLR |
>> +               S3C64XX_SDHCI_CTRL2_ENCMDCNFMSK |
>> +               S3C_SDHCI_CTRL2_ENFBCLKRX |
>> +               S3C_SDHCI_CTRL2_DFCNT_NONE |
>> +               S3C_SDHCI_CTRL2_ENCLKOUTHOLD);
>> +     writel(ctrl, host->ioaddr + S3C_SDHCI_CONTROL2);
>> +
>> +     /* reconfigure the controller for new clock rate */
>> +     ctrl = (S3C_SDHCI_CTRL3_FCSEL1 | S3C_SDHCI_CTRL3_FCSEL0);
>> +     if (clock < 25 * 1000000)
>> +             ctrl |= (S3C_SDHCI_CTRL3_FCSEL3 |
>> S3C_SDHCI_CTRL3_FCSEL2);
>> +     writel(ctrl, host->ioaddr + S3C_SDHCI_CONTROL3);
>>  }
>>
>>  /**
>> --
>
> Basically, it's good to move common codes and to remove that. But I'm not
> sure we don't _really_ need to keep SoC specific control function such as
> cfg_card().

The existing cfg_card callback functions have the same functionality
except for setting the clock out drive strength on those SoC's which
do not support this feature. If writing to those reserved bits does
not cause any issues, then the cfg_card function can be made common to
all Samsung SoC's.

Since the functionality of cfg_card callback is SoC specific and not
board specific, there is another way of doing this. The id_table
member could be added to the platform_driver structure in sdhci-s3c
driver. Each SoC platform code could set the name of the sdhci device
during initialization and there could be SoC specific cfg_card
functions included in the sdhci-s3c driver itself. This would be
required only if there is a problem by writing to the reserved bits in
control4 register. But testing does not show any issues writing to
these reserved bits.

The main intention of moving the cfg_card function from platform code
to driver code is to remove the callback function pointer from the
sdhci driver platform data which is very important to get full device
tree support for sdhci-s3c driver.

Thanks for your review and comments.

Regards,
Thomas.

>
> Thanks.
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index 2bd7bf4..d891682 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -203,17 +203,23 @@  static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock)
 		writel(ctrl, host->ioaddr + S3C_SDHCI_CONTROL2);
 	}
 
-	/* reconfigure the hardware for new clock rate */
-
-	{
-		struct mmc_ios ios;
-
-		ios.clock = clock;
-
-		if (ourhost->pdata->cfg_card)
-			(ourhost->pdata->cfg_card)(ourhost->pdev, host->ioaddr,
-						   &ios, NULL);
-	}
+	/* reprogram default hardware configuration */
+	writel(S3C64XX_SDHCI_CONTROL4_DRIVE_9mA,
+		host->ioaddr + S3C64XX_SDHCI_CONTROL4);
+
+	ctrl = readl(host->ioaddr + S3C_SDHCI_CONTROL2);
+	ctrl |= (S3C64XX_SDHCI_CTRL2_ENSTAASYNCCLR |
+		  S3C64XX_SDHCI_CTRL2_ENCMDCNFMSK |
+		  S3C_SDHCI_CTRL2_ENFBCLKRX |
+		  S3C_SDHCI_CTRL2_DFCNT_NONE |
+		  S3C_SDHCI_CTRL2_ENCLKOUTHOLD);
+	writel(ctrl, host->ioaddr + S3C_SDHCI_CONTROL2);
+
+	/* reconfigure the controller for new clock rate */
+	ctrl = (S3C_SDHCI_CTRL3_FCSEL1 | S3C_SDHCI_CTRL3_FCSEL0);
+	if (clock < 25 * 1000000)
+		ctrl |= (S3C_SDHCI_CTRL3_FCSEL3 | S3C_SDHCI_CTRL3_FCSEL2);
+	writel(ctrl, host->ioaddr + S3C_SDHCI_CONTROL3);
 }
 
 /**