Hi Mr. Park,
On 4 October 2011 13:23, Kyungmin Park <kmpark@xxxxxxxxxxxxx> wrote:
> Hi,
>
> On Tue, Oct 4, 2011 at 4:28 PM, Thomas Abraham
> <thomas.abraham@xxxxxxxxxx> wrote:
>> SDHCI controllers on Exynos4 do not include the sdclk divider as per the
>> sdhci controller specification. This case can be represented using the
>> sdhci quirk SDHCI_QUIRK_NONSTANDARD_CLOCK instead of using an additional
>> enum type definition 'clk_types'.
>>
>> Hence, usage of clk_type member in platform data is removed and the sdhci
>> quirk is used. In addition to that, since this qurik is SoC specific,
>> driver data is introduced to represent controllers on SoC's that require
>> this quirk.
> Right.
>>
>> Cc: Ben Dooks <ben-linux@xxxxxxxxx>
>> Cc: Jeongbae Seo <jeongbae.seo@xxxxxxxxxxx>
>> Signed-off-by: Thomas Abraham <thomas.abraham@xxxxxxxxxx>
>> ---
>> In the function 'sdhci_cmu_set_clock', the code that waits for a stable
>> sdclk clock is replicated from 'sdhci_set_clock' function. This portion
>> of code is replicated in sdhci-cns3xxx driver as well. This duplication
>> could have been avoid by adding a new function in sdhci host controller
>> driver that waits for stable sdclk and sdhci-s3c/sdhci-cns3xxx can be
>> the users of the new function. If adding a new function in sdhci host
>> controller driver is acceptable, an additional patch in this series
>> can be added that creates the new function that waits for stable sdclk.
>>
>> drivers/mmc/host/sdhci-s3c.c | 74 +++++++++++++++++++++++++++++++++++++++--
>> 1 files changed, 70 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
>> index 82709b6..b29e734 100644
>> --- a/drivers/mmc/host/sdhci-s3c.c
>> +++ b/drivers/mmc/host/sdhci-s3c.c
>> @@ -53,6 +53,18 @@ struct sdhci_s3c {
>> struct clk *clk_bus[MAX_BUS_CLK];
>> };
>>
>> +/**
>> + * struct sdhci_s3c_driver_data - S3C SDHCI platform specific driver data
>> + * @sdhci_quirks: sdhci host specific quirks.
>> + *
>> + * Specifies platform specific configuration of sdhci controller.
>> + * Note: A structure for driver specific platform data is used for future
>> + * expansion of its usage.
>> + */
>> +struct sdhci_s3c_drv_data {
>> + unsigned int sdhci_quirks;
>> +};
>> +
>> static inline struct sdhci_s3c *to_s3c(struct sdhci_host *host)
>> {
>> return sdhci_priv(host);
>> @@ -132,10 +144,10 @@ static unsigned int sdhci_s3c_consider_clock(struct sdhci_s3c *ourhost,
>> return UINT_MAX;
>>
>> /*
>> - * Clock divider's step is different as 1 from that of host controller
>> - * when 'clk_type' is S3C_SDHCI_CLK_DIV_EXTERNAL.
>> + * If controller uses a non-standard clock division, find the best clock
>> + * speed possible with selected clock source and skip the division.
>> */
>> - if (ourhost->pdata->clk_type) {
>> + if (ourhost->host->quirks & SDHCI_QUIRK_NONSTANDARD_CLOCK) {
>> rate = clk_round_rate(clksrc, wanted);
>> return wanted - rate;
>> }
>> @@ -272,6 +284,8 @@ static unsigned int sdhci_cmu_get_min_clock(struct sdhci_host *host)
>> static void sdhci_cmu_set_clock(struct sdhci_host *host, unsigned int clock)
>> {
>> struct sdhci_s3c *ourhost = to_s3c(host);
>> + unsigned long timeout;
>> + u16 clk = 0;
>>
>> /* don't bother if the clock is going off */
>> if (clock == 0)
>> @@ -282,6 +296,25 @@ static void sdhci_cmu_set_clock(struct sdhci_host *host, unsigned int clock)
>> clk_set_rate(ourhost->clk_bus[ourhost->cur_clk], clock);
>>
>> host->clock = clock;
>> +
>> + clk = SDHCI_CLOCK_INT_EN;
>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>> +
>> + /* Wait max 20 ms */
>> + timeout = 20;
>> + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>> + & SDHCI_CLOCK_INT_STABLE)) {
>> + if (timeout == 0) {
>> + printk(KERN_ERR "%s: Internal clock never "
>> + "stabilised.\n", mmc_hostname(host->mmc));
>> + return;
>> + }
>> + timeout--;
>> + mdelay(1);
>> + }
>> +
>> + clk |= SDHCI_CLOCK_CARD_EN;
>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>> }
>>
>> /**
>> @@ -382,9 +415,17 @@ static void sdhci_s3c_setup_card_detect_gpio(struct sdhci_s3c *sc)
>> }
>> }
>>
>> +static inline struct sdhci_s3c_drv_data *sdhci_s3c_get_driver_data(
>> + struct platform_device *pdev)
>> +{
>> + return (struct sdhci_s3c_drv_data *)
>> + platform_get_device_id(pdev)->driver_data;
>> +}
>> +
>> static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>> {
>> struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
>> + struct sdhci_s3c_drv_data *drv_data;
>> struct device *dev = &pdev->dev;
>> struct sdhci_host *host;
>> struct sdhci_s3c *sc;
>> @@ -414,6 +455,7 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>> return PTR_ERR(host);
>> }
>>
>> + drv_data = sdhci_s3c_get_driver_data(pdev);
>> sc = sdhci_priv(host);
>>
>> sc->host = host;
>> @@ -494,6 +536,8 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>> /* Setup quirks for the controller */
>> host->quirks |= SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC;
>> host->quirks |= SDHCI_QUIRK_NO_HISPD_BIT;
>> + if (drv_data)
>> + host->quirks |= drv_data->sdhci_quirks;
>>
>> #ifndef CONFIG_MMC_SDHCI_S3C_DMA
>>
>> @@ -534,7 +578,7 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>> * If controller does not have internal clock divider,
>> * we can use overriding functions instead of default.
>> */
>> - if (pdata->clk_type) {
>> + if (host->quirks & SDHCI_QUIRK_NONSTANDARD_CLOCK) {
>> sdhci_s3c_ops.set_clock = sdhci_cmu_set_clock;
>> sdhci_s3c_ops.get_min_clock = sdhci_cmu_get_min_clock;
>> sdhci_s3c_ops.get_max_clock = sdhci_cmu_get_max_clock;
>> @@ -639,9 +683,31 @@ static int sdhci_s3c_resume(struct platform_device *dev)
>> #define sdhci_s3c_resume NULL
>> #endif
>>
>> +#if defined(CONFIG_CPU_EXYNOS4210) || defined(CONFIG_SOC_EXYNOS4212)
>> +static struct sdhci_s3c_drv_data exynos4_sdhci_drv_data = {
>> + .sdhci_quirks = SDHCI_QUIRK_NONSTANDARD_CLOCK,
>> +};
>> +#define EXYNOS4_SDHCI_DRV_DATA ((kernel_ulong_t)&exynos4_sdhci_drv_data)
>> +#else
>> +#define EXYNOS4_SDHCI_DRV_DATA (kernel_ulong_t)NULL
>> +#endif
> It's too small to save the memory. I think no need to guard the exynos
> driver data with ifdef/endif.
There could be other elements added to the 'sdhci_s3c_drv_data'
structure as new variants of sdhci-s3c controller would appear in
future SoC. I am not aware of any such variations in the upcoming
SoC's but this was done taking into consideration that there could be
slight variations which the driver would have to support.
>> +
>> +static struct platform_device_id sdhci_s3c_driver_ids[] = {
>> + {
>> + .name = "s3c-sdhci",
>> + .driver_data = (kernel_ulong_t)NULL,
>> + },
>> + {
>> + .name = "exynos4-sdhci",
>> + .driver_data = EXYNOS4_SDHCI_DRV_DATA,
>> + },
>> +};
>> +MODULE_DEVICE_TABLE(platform, sdhci_s3c_driver_ids);
>
> Do you have a plan to use the sdhci-pltfm.c as other drivers did?
> some drivers are already use it and support OF features.
I did check few months back if sdhci-pltfm.c can be used for sdhci-s3c
drivers but due to slightly complex operations which sdhci-s3c
handles, I did not try to base samsung sdhci driver support on
sdhci-pltfm.c. So, at present, I do not have a plan to move to
sdhci-pltfm.c.
>
> Thank you,
> Kyungmin Park
Thanks for your review and comments.
Regards,
Thomas.
>> +
>> static struct platform_driver sdhci_s3c_driver = {
>> .probe = sdhci_s3c_probe,
>> .remove = __devexit_p(sdhci_s3c_remove),
>> + .id_table = sdhci_s3c_driver_ids,
>> .suspend = sdhci_s3c_suspend,
>> .resume = sdhci_s3c_resume,
>> .driver = {
>> --
>> 1.6.6.rc2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
