[PATCH] sdhci-s3c: support non-standard clock setting for c210

[PATCH] sdhci-s3c: support non-standard clock setting for c210

[Kukjin Kim]

Kyungmin Park wrote:
> 
> On Thu, Sep 2, 2010 at 7:20 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > Jaehoon Chung wrote:
> >>
> >> This is sdhci-s3c patch for c210.
> >> c210 didn't use divider of host controller.
> >>
> >> Host Controller need other clock setting methods.
> >>
> >> So I add the callback functions for s5pc210.
> >> also I set 400KHz for initial clock.
> >>
> >> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> >> ?Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
   ^^^
Unnecessary whitespace

If your patch includes arch/arm stuff, please add Linux-arm-kernel
maillinglist.
(Cc'ed that)

> >>
> >> ---
> >> ?arch/arm/plat-samsung/include/plat/sdhci.h | ? 19 ++++++++
> >> ?drivers/mmc/host/sdhci-s3c.c ? ? ? ? ? ? ? | ? 68
> >> ++++++++++++++++++++++++++++
> >> ?2 files changed, 87 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/arm/plat-samsung/include/plat/sdhci.h
b/arch/arm/plat-
> >> samsung/include/plat/sdhci.h
> >> index 30844c2..7c75ee3 100644
> >> --- a/arch/arm/plat-samsung/include/plat/sdhci.h
> >> +++ b/arch/arm/plat-samsung/include/plat/sdhci.h
> >> @@ -15,6 +15,8 @@
> >> ?#ifndef __PLAT_S3C_SDHCI_H
> >> ?#define __PLAT_S3C_SDHCI_H __FILE__
> >>
> >> +#include <plat/devs.h>
> >> +
> >> ?struct platform_device;
> >> ?struct mmc_host;
> >> ?struct mmc_card;
> >> @@ -288,4 +290,21 @@ static inline void s5pv210_default_sdhci3(void) {
}
> >>
> >> ?#endif /* CONFIG_S5PV210_SETUP_SDHCI */
> >>
> >> +/* re-define device name depending on support. */
> >> +static inline void s3c_hsmmc_setname(char *name)
> >> +{
> >> +#ifdef CONFIG_S3C_DEV_HSMMC
> >> + ? ? s3c_device_hsmmc0.name = name;
> >> +#endif
> >> +#ifdef CONFIG_S3C_DEV_HSMMC1
> >> + ? ? s3c_device_hsmmc1.name = name;
> >> +#endif
> >> +#ifdef CONFIG_S3C_DEV_HSMMC2
> >> + ? ? s3c_device_hsmmc2.name = name;
> >> +#endif
> >> +#ifdef CONFIG_S3C_DEV_HSMMC3
> >> + ? ? s3c_device_hsmmc3.name = name;
> >> +#endif
> >> +}
> >> +
> >> ?#endif /* __PLAT_S3C_SDHCI_H */
> >
> > It would be helpful to me if you could separate platform and driver
patches.
> > And if you want to add s3c_hsmmc_setname(), please send together code
that
> > it is used.
> >
> >> diff --git a/drivers/mmc/host/sdhci-s3c.c
b/drivers/mmc/host/sdhci-s3c.c
> >> index 71ad416..3927793 100644
> >> --- a/drivers/mmc/host/sdhci-s3c.c
> >> +++ b/drivers/mmc/host/sdhci-s3c.c

(snip)

> >
> > How do you think about using quirk to separate S5PV310 case as
following?
> > I think this is more general method in here...And will be submitted soon
> > after fixing something.
> What's the meaning of "general method"?
> 
I'd like to ask you why there is quirk in sdmmc driver.

> and how/where do you set the host->quirks? when board or platform set
> the these quirks? who know it uses nonstandard quirk?

How about following?

static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
...
+	if (pdev->id_entry->driver_data == TYPE_S5PV310)
+		host->quirks |= SDHCI_QUIRK_BROKEN_CLOCK_DIVIDER;
...

> In case s5pc210 don't have standard host controller. it's more clear
> to use own functions instead of quirks.
> 
Why do we add another callback function?

> >
> > From: Hyuk Lee <hyuk1.lee@samsung.com>
> >
> > diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> > index 71ad416..1ac2f36 100644
> > --- a/drivers/mmc/host/sdhci-s3c.c
> > +++ b/drivers/mmc/host/sdhci-s3c.c

(snip)

> > @@ -221,6 +257,11 @@ static unsigned int sdhci_s3c_get_min_clock(struct
> > sdhci_host *host)
> > ? ? ? ?unsigned int delta, min = UINT_MAX;
> > ? ? ? ?int src;
> >
> > + ? ? ? /* There is only one clock source(sclk) if there is no clock
divider
> > + ? ? ? ?* in the host controller */
> > + ? ? ? if(host->quirks & SDHCI_QUIRK_BROKEN_CLOCK_DIVIDER)
> > + ? ? ? ? ? ? ? return clk_round_rate(ourhost->clk_bus[2], 400000);
> what's the clk_bus[2]?
> 
Should be clk_bus[ourhost->cur_clk]

(snip)

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

[PATCH] sdhci-s3c: support non-standard clock setting for c210

[Kukjin Kim]

Kukjin Kim wrote:
> 
> Kyungmin Park wrote:
> >
> > On Thu, Sep 2, 2010 at 7:20 PM, Kukjin Kim <kgene.kim@samsung.com>
wrote:
> > > Jaehoon Chung wrote:
> > >>
> > >> This is sdhci-s3c patch for c210.
> > >> c210 didn't use divider of host controller.
> > >>
> > >> Host Controller need other clock setting methods.
> > >>
> > >> So I add the callback functions for s5pc210.
> > >> also I set 400KHz for initial clock.
> > >>
> > >> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> > >> ?Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>    ^^^
> Unnecessary whitespace
> 
> If your patch includes arch/arm stuff, please add Linux-arm-kernel
maillinglist.
> (Cc'ed that)
> 
> > >>
> > >> ---
> > >> ?arch/arm/plat-samsung/include/plat/sdhci.h | ? 19 ++++++++
> > >> ?drivers/mmc/host/sdhci-s3c.c ? ? ? ? ? ? ? | ? 68
> > >> ++++++++++++++++++++++++++++
> > >> ?2 files changed, 87 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/arch/arm/plat-samsung/include/plat/sdhci.h
b/arch/arm/plat-
> > >> samsung/include/plat/sdhci.h
> > >> index 30844c2..7c75ee3 100644
> > >> --- a/arch/arm/plat-samsung/include/plat/sdhci.h
> > >> +++ b/arch/arm/plat-samsung/include/plat/sdhci.h

(snip)

> > >> diff --git a/drivers/mmc/host/sdhci-s3c.c
b/drivers/mmc/host/sdhci-s3c.c
> > >> index 71ad416..3927793 100644
> > >> --- a/drivers/mmc/host/sdhci-s3c.c
> > >> +++ b/drivers/mmc/host/sdhci-s3c.c
> 
> (snip)
> 
> > >
> > > How do you think about using quirk to separate S5PV310 case as
following?
> > > I think this is more general method in here...And will be submitted
soon
> > > after fixing something.

mm...I will think about your comments again. Then let's discuss about this
issue.

Thanks.

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

> > What's the meaning of "general method"?
> >
> I'd like to ask you why there is quirk in sdmmc driver.
> 
> > and how/where do you set the host->quirks? when board or platform set
> > the these quirks? who know it uses nonstandard quirk?
> 
> How about following?
> 
> static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
> ...
> +	if (pdev->id_entry->driver_data == TYPE_S5PV310)
> +		host->quirks |= SDHCI_QUIRK_BROKEN_CLOCK_DIVIDER;
> ...
> 
> > In case s5pc210 don't have standard host controller. it's more clear
> > to use own functions instead of quirks.
> >
> Why do we add another callback function?
> 
> > >
> > > From: Hyuk Lee <hyuk1.lee@samsung.com>
> > >
> > > diff --git a/drivers/mmc/host/sdhci-s3c.c
b/drivers/mmc/host/sdhci-s3c.c
> > > index 71ad416..1ac2f36 100644
> > > --- a/drivers/mmc/host/sdhci-s3c.c
> > > +++ b/drivers/mmc/host/sdhci-s3c.c
> 
> (snip)
> 
> > > @@ -221,6 +257,11 @@ static unsigned int
sdhci_s3c_get_min_clock(struct
> > > sdhci_host *host)
> > > ? ? ? ?unsigned int delta, min = UINT_MAX;
> > > ? ? ? ?int src;
> > >
> > > + ? ? ? /* There is only one clock source(sclk) if there is no clock
divider
> > > + ? ? ? ?* in the host controller */
> > > + ? ? ? if(host->quirks & SDHCI_QUIRK_BROKEN_CLOCK_DIVIDER)
> > > + ? ? ? ? ? ? ? return clk_round_rate(ourhost->clk_bus[2], 400000);
> > what's the clk_bus[2]?
> >
> Should be clk_bus[ourhost->cur_clk]
> 
> (snip)
> 

[PATCH] sdhci-s3c: support non-standard clock setting for c210

[Kyungmin Park]

On Thu, Sep 2, 2010 at 7:47 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Kyungmin Park wrote:
>>
>> On Thu, Sep 2, 2010 at 7:20 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> > Jaehoon Chung wrote:
>> >>
>> >> This is sdhci-s3c patch for c210.
>> >> c210 didn't use divider of host controller.
>> >>
>> >> Host Controller need other clock setting methods.
>> >>
>> >> So I add the callback functions for s5pc210.
>> >> also I set 400KHz for initial clock.
>> >>
>> >> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>> >> ?Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ? ^^^
> Unnecessary whitespace
>
> If your patch includes arch/arm stuff, please add Linux-arm-kernel
> maillinglist.
> (Cc'ed that)
>
>> >>
>> >> ---
>> >> ?arch/arm/plat-samsung/include/plat/sdhci.h | ? 19 ++++++++
>> >> ?drivers/mmc/host/sdhci-s3c.c ? ? ? ? ? ? ? | ? 68
>> >> ++++++++++++++++++++++++++++
>> >> ?2 files changed, 87 insertions(+), 0 deletions(-)
>> >>
>> >> diff --git a/arch/arm/plat-samsung/include/plat/sdhci.h
> b/arch/arm/plat-
>> >> samsung/include/plat/sdhci.h
>> >> index 30844c2..7c75ee3 100644
>> >> --- a/arch/arm/plat-samsung/include/plat/sdhci.h
>> >> +++ b/arch/arm/plat-samsung/include/plat/sdhci.h
>> >> @@ -15,6 +15,8 @@
>> >> ?#ifndef __PLAT_S3C_SDHCI_H
>> >> ?#define __PLAT_S3C_SDHCI_H __FILE__
>> >>
>> >> +#include <plat/devs.h>
>> >> +
>> >> ?struct platform_device;
>> >> ?struct mmc_host;
>> >> ?struct mmc_card;
>> >> @@ -288,4 +290,21 @@ static inline void s5pv210_default_sdhci3(void) {
> }
>> >>
>> >> ?#endif /* CONFIG_S5PV210_SETUP_SDHCI */
>> >>
>> >> +/* re-define device name depending on support. */
>> >> +static inline void s3c_hsmmc_setname(char *name)
>> >> +{
>> >> +#ifdef CONFIG_S3C_DEV_HSMMC
>> >> + ? ? s3c_device_hsmmc0.name = name;
>> >> +#endif
>> >> +#ifdef CONFIG_S3C_DEV_HSMMC1
>> >> + ? ? s3c_device_hsmmc1.name = name;
>> >> +#endif
>> >> +#ifdef CONFIG_S3C_DEV_HSMMC2
>> >> + ? ? s3c_device_hsmmc2.name = name;
>> >> +#endif
>> >> +#ifdef CONFIG_S3C_DEV_HSMMC3
>> >> + ? ? s3c_device_hsmmc3.name = name;
>> >> +#endif
>> >> +}
>> >> +
>> >> ?#endif /* __PLAT_S3C_SDHCI_H */
>> >
>> > It would be helpful to me if you could separate platform and driver
> patches.
>> > And if you want to add s3c_hsmmc_setname(), please send together code
> that
>> > it is used.
>> >
>> >> diff --git a/drivers/mmc/host/sdhci-s3c.c
> b/drivers/mmc/host/sdhci-s3c.c
>> >> index 71ad416..3927793 100644
>> >> --- a/drivers/mmc/host/sdhci-s3c.c
>> >> +++ b/drivers/mmc/host/sdhci-s3c.c
>
> (snip)
>
>> >
>> > How do you think about using quirk to separate S5PV310 case as
> following?
>> > I think this is more general method in here...And will be submitted soon
>> > after fixing something.
>> What's the meaning of "general method"?
>>
> I'd like to ask you why there is quirk in sdmmc driver.
>
>> and how/where do you set the host->quirks? when board or platform set
>> the these quirks? who know it uses nonstandard quirk?
>
> How about following?
>
> static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
> ...
> + ? ? ? if (pdev->id_entry->driver_data == TYPE_S5PV310)
> + ? ? ? ? ? ? ? host->quirks |= SDHCI_QUIRK_BROKEN_CLOCK_DIVIDER;

What's the difference override the functions?

> ...
>
>> In case s5pc210 don't have standard host controller. it's more clear
>> to use own functions instead of quirks.
>>
> Why do we add another callback function?

It's override functions. See the patch posted.

Usually you insist use your code. then send it early to mailing list
and get the feedback.

Thank you,
Kyungmin Park

>
>> >
>> > From: Hyuk Lee <hyuk1.lee@samsung.com>
>> >
>> > diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
>> > index 71ad416..1ac2f36 100644
>> > --- a/drivers/mmc/host/sdhci-s3c.c
>> > +++ b/drivers/mmc/host/sdhci-s3c.c
>
> (snip)
>
>> > @@ -221,6 +257,11 @@ static unsigned int sdhci_s3c_get_min_clock(struct
>> > sdhci_host *host)
>> > ? ? ? ?unsigned int delta, min = UINT_MAX;
>> > ? ? ? ?int src;
>> >
>> > + ? ? ? /* There is only one clock source(sclk) if there is no clock
> divider
>> > + ? ? ? ?* in the host controller */
>> > + ? ? ? if(host->quirks & SDHCI_QUIRK_BROKEN_CLOCK_DIVIDER)
>> > + ? ? ? ? ? ? ? return clk_round_rate(ourhost->clk_bus[2], 400000);
>> what's the clk_bus[2]?
>>
> Should be clk_bus[ourhost->cur_clk]
>
> (snip)
>
> 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 at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

[PATCH] sdhci-s3c: support non-standard clock setting for c210

[Jaehoon Chung]

Kukjin Kim wrote:
> Kyungmin Park wrote:
>   
>> On Thu, Sep 2, 2010 at 7:20 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>>     
>>> Jaehoon Chung wrote:
>>>       
>>>> This is sdhci-s3c patch for c210.
>>>> c210 didn't use divider of host controller.
>>>>
>>>> Host Controller need other clock setting methods.
>>>>
>>>> So I add the callback functions for s5pc210.
>>>> also I set 400KHz for initial clock.
>>>>
>>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>>>  Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>         
>    ^^^
> Unnecessary whitespace
>
> If your patch includes arch/arm stuff, please add Linux-arm-kernel
> maillinglist.
> (Cc'ed that)
>
>   
i will send to Linux-arm-kernel mailing list.
If you want to test this patch. you also applied following patch.
(ARM: Samsung: Fix header double inclusion)

diff --git a/arch/arm/plat-samsung/include/plat/devs.h b/arch/arm/plat-samsung/include/plat/devs.h
index 85f6f23..42581f8 100644
--- a/arch/arm/plat-samsung/include/plat/devs.h
+++ b/arch/arm/plat-samsung/include/plat/devs.h
@@ -8,7 +8,11 @@
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
-*/
+ */
+
+#ifndef __PLAT_SAMSUNG_DEVS_H
+#define __PLAT_SAMSUNG_DEVS_H
+
 #include <linux/platform_device.h>
 
 struct s3c24xx_uart_resources {
@@ -128,3 +132,5 @@ extern struct platform_device s3c_device_ac97;
  */
 extern void *s3c_set_platdata(void *pd, size_t pdsize,
 			      struct platform_device *pdev);
+
+#endif

And s3c_hsmmc_setname() function could be used in cpu.c file

Thanks
Jaehoon Chung
>>>> ---
>>>>  arch/arm/plat-samsung/include/plat/sdhci.h |   19 ++++++++
>>>>  drivers/mmc/host/sdhci-s3c.c               |   68
>>>> ++++++++++++++++++++++++++++
>>>>  2 files changed, 87 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/arch/arm/plat-samsung/include/plat/sdhci.h
>>>>         
> b/arch/arm/plat-
>   
>>>> samsung/include/plat/sdhci.h
>>>> index 30844c2..7c75ee3 100644
>>>> --- a/arch/arm/plat-samsung/include/plat/sdhci.h
>>>> +++ b/arch/arm/plat-samsung/include/plat/sdhci.h
>>>> @@ -15,6 +15,8 @@
>>>>  #ifndef __PLAT_S3C_SDHCI_H
>>>>  #define __PLAT_S3C_SDHCI_H __FILE__
>>>>
>>>> +#include <plat/devs.h>
>>>> +
>>>>  struct platform_device;
>>>>  struct mmc_host;
>>>>  struct mmc_card;
>>>> @@ -288,4 +290,21 @@ static inline void s5pv210_default_sdhci3(void) {
>>>>         
> }
>   
>>>>  #endif /* CONFIG_S5PV210_SETUP_SDHCI */
>>>>
>>>> +/* re-define device name depending on support. */
>>>> +static inline void s3c_hsmmc_setname(char *name)
>>>> +{
>>>> +#ifdef CONFIG_S3C_DEV_HSMMC
>>>> +     s3c_device_hsmmc0.name = name;
>>>> +#endif
>>>> +#ifdef CONFIG_S3C_DEV_HSMMC1
>>>> +     s3c_device_hsmmc1.name = name;
>>>> +#endif
>>>> +#ifdef CONFIG_S3C_DEV_HSMMC2
>>>> +     s3c_device_hsmmc2.name = name;
>>>> +#endif
>>>> +#ifdef CONFIG_S3C_DEV_HSMMC3
>>>> +     s3c_device_hsmmc3.name = name;
>>>> +#endif
>>>> +}
>>>> +
>>>>  #endif /* __PLAT_S3C_SDHCI_H */
>>>>         
>>> It would be helpful to me if you could separate platform and driver
>>>       
> patches.
>   
i will seperate platform and driver patches, then i will resend to
mailing list.
>>> And if you want to add s3c_hsmmc_setname(), please send together code
>>>       
> that
>   
>>> it is used.
>>>
>>>       
>>>> diff --git a/drivers/mmc/host/sdhci-s3c.c
>>>>         
> b/drivers/mmc/host/sdhci-s3c.c
>   
>>>> index 71ad416..3927793 100644
>>>> --- a/drivers/mmc/host/sdhci-s3c.c
>>>> +++ b/drivers/mmc/host/sdhci-s3c.c
>>>>         
>
> (snip)
>
>   
>>> How do you think about using quirk to separate S5PV310 case as
>>>       
> following?
>   
>>> I think this is more general method in here...And will be submitted soon
>>> after fixing something.
>>>       
>> What's the meaning of "general method"?
>>
>>     
> I'd like to ask you why there is quirk in sdmmc driver.
>
>   
>> and how/where do you set the host->quirks? when board or platform set
>> the these quirks? who know it uses nonstandard quirk?
>>     
>
> How about following?
>
> static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
> ...
> +	if (pdev->id_entry->driver_data == TYPE_S5PV310)
> +		host->quirks |= SDHCI_QUIRK_BROKEN_CLOCK_DIVIDER;
> ...
>
>   
>> In case s5pc210 don't have standard host controller. it's more clear
>> to use own functions instead of quirks.
>>
>>     
> Why do we add another callback function?
>
>   
>>> From: Hyuk Lee <hyuk1.lee@samsung.com>
>>>
>>> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
>>> index 71ad416..1ac2f36 100644
>>> --- a/drivers/mmc/host/sdhci-s3c.c
>>> +++ b/drivers/mmc/host/sdhci-s3c.c
>>>       
>
> (snip)
>
>   
>>> @@ -221,6 +257,11 @@ static unsigned int sdhci_s3c_get_min_clock(struct
>>> sdhci_host *host)
>>>        unsigned int delta, min = UINT_MAX;
>>>        int src;
>>>
>>> +       /* There is only one clock source(sclk) if there is no clock
>>>       
> divider
>   
>>> +        * in the host controller */
>>> +       if(host->quirks & SDHCI_QUIRK_BROKEN_CLOCK_DIVIDER)
>>> +               return clk_round_rate(ourhost->clk_bus[2], 400000);
>>>       
>> what's the clk_bus[2]?
>>
>>     
> Should be clk_bus[ourhost->cur_clk]
>
> (snip)
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
>
>