Fwd: can we remove the quirk SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN ?

Fwd: can we remove the quirk SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN ?

[Philip Rakity]

reposting since we are discussing removing quirks.   

We could partition the quirks into one set for controller quirks and one set for platform quirks.  That would give us another 32 quirks. 

Philip

Begin forwarded message:

> From: Philip Rakity <prakity@marvell.com>
> Date: September 24, 2010 2:31:25 AM PDT
> To: Anton Vorontsov <cbouatmailru@gmail.com>
> Subject: Re: can we remove the quirk SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN ?
> 
> 
> you are right -- it would work -- was trying to get rid of quirk since really not needed.  Do not agree with Pierre.  Understand when registers in sdhci.c need changes quirks are good.  getting valves from platform code seems the wrong usage model.
> 
> On Sep 24, 2010, at 2:27 AM, Anton Vorontsov wrote:
> 
>> On Fri, Sep 24, 2010 at 02:15:57AM -0700, Philip Rakity wrote:
>>> 
>>> For our controller the original code would not work, since we get a clock value but the clock value may not be right.  The chip sd bus frequency can be adjusted in the platform init code to a higher value.  This value needs to be communicated to the sd layer.
>>> 
>>> Are you happy that the patch does not break your code?  If so I can submit patch with extended rationale.
>> 
>> Hm. Why would the driver won't work if you do
>> specify SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN? Or there's some additional
>> patch that you're talking about?
>> 
>> Thanks,
>> 
>>> 
>>> Philip
>>> 
>>> Eg
>>> ________________________________________
>>> From: Anton Vorontsov [cbouatmailru@gmail.com]
>>> Sent: Thursday, September 23, 2010 11:59 PM
>>> To: Philip Rakity
>>> Subject: Re: can we remove the quirk  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN ?
>>> 
>>> On Thu, Sep 23, 2010 at 04:57:41PM -0700, Philip Rakity wrote:
>>>> 
>>>> would be much better if I enclosed the code that compiles.
>>>> 
>>>> Philip
>>>> 
>>>> ====
>>>> 
>>>> I was looking at my code and then realized  the quirk is not needed since host->ops->max_clock is REALLY the test.
>>>> 
>>>> Do you concur ?  if so -- want to co sponsor the patch
>>>> 
>>>> I have NOT tried it since do not have your system.
>>> 
>>> See why there is a quirk:
>>> 
>>> http://kerneltrap.org/mailarchive/linux-kernel/2010/2/19/4540115
>>> 
>>> Thanks,
>>> 
>>>> Philip
>>>> 
>>>> ====
>>>> 
>>>> b/drivers/mmc/host/sdhci-cns3xxx.c
>>>> index b7050b3..27c896d 100644
>>>> --- a/drivers/mmc/host/sdhci-cns3xxx.c
>>>> +++ b/drivers/mmc/host/sdhci-cns3xxx.c
>>>> @@ -91,7 +91,6 @@ struct sdhci_pltfm_data sdhci_cns3xxx_pdata = {
>>>>     .quirks = SDHCI_QUIRK_BROKEN_DMA |
>>>>               SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK |
>>>>               SDHCI_QUIRK_INVERTED_WRITE_PROTECT |
>>>> -               SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
>>>>               SDHCI_QUIRK_BROKEN_TIMEOUT_VAL |
>>>>               SDHCI_QUIRK_NONSTANDARD_CLOCK,
>>>> };
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index 401527d..3d809f5 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -1779,18 +1779,20 @@ int sdhci_add_host(struct sdhci_host *host)
>>>>             mmc_dev(host->mmc)->dma_mask = &host->dma_mask;
>>>>     }
>>>> 
>>>> -     host->max_clk =
>>>> -             (caps & SDHCI_CLOCK_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
>>>> -     host->max_clk *= 1000000;
>>>> -     if (host->max_clk == 0 || host->quirks &
>>>> -                     SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN) {
>>>> -             if (!host->ops->get_max_clock) {
>>>> -                     printk(KERN_ERR
>>>> -                            "%s: Hardware doesn't specify base clock "
>>>> -                            "frequency.\n", mmc_hostname(mmc));
>>>> -                     return -ENODEV;
>>>> -             }
>>>> +     if (host->ops->get_max_clock)
>>>>             host->max_clk = host->ops->get_max_clock(host);
>>>> +     else {
>>>> +             host->max_clk =
>>>> +                     (caps & SDHCI_CLOCK_BASE_MASK)
>>>> +                             >> SDHCI_CLOCK_BASE_SHIFT;
>>>> +             host->max_clk *= 1000000;
>>>> +     }
>>>> +
>>>> +     if (host->max_clk == 0) {
>>>> +             printk(KERN_ERR
>>>> +                    "%s: Hardware doesn't specify base clock "
>>>> +                    "frequency.\n", mmc_hostname(mmc));
>>>> +             return -ENODEV;
>>>>     }
>>>> 
>>>>     host->timeout_clk =
>>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>>> index d316bc7..d7c85fb 100644
>>>> --- a/drivers/mmc/host/sdhci.h
>>>> +++ b/drivers/mmc/host/sdhci.h
>>>> @@ -237,8 +237,6 @@ struct sdhci_host {
>>>> #define SDHCI_QUIRK_DELAY_AFTER_POWER                        (1<<23)
>>>> /* Controller uses SDCLK instead of TMCLK for data timeouts */
>>>> #define SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK          (1<<24)
>>>> -/* Controller reports wrong base clock capability */
>>>> -#define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN            (1<<25)
>>>> /* Controller cannot support End Attribute in NOP ADMA descriptor */
>>>> #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC            (1<<26)
>>>> /* Controller is missing device caps. Use caps provided by host */
>>>> 
>>> 
>>> --
>>> Anton Vorontsov
>>> email: cbouatmailru@gmail.com
>>> irc://irc.freenode.net/bd2
>> 
>> -- 
>> Anton Vorontsov
>> email: cbouatmailru@gmail.com
>> irc://irc.freenode.net/bd2
> 

Re: Fwd: can we remove the quirk SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN ?

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

On Tue, Nov 09, 2010 at 03:45:15AM -0800, Philip Rakity wrote:
> 
> reposting since we are discussing removing quirks.   
> 
> We could partition the quirks into one set for controller quirks and one set for platform quirks.  That would give us another 32 quirks. 

IMHO we need less quirks, not more. Adding the number of potential
quirks just delays the underlying problem. Also, unless someone
enlightens me, I don't think there are platform quirks, only
platform_data?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: can we remove the quirk SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN ?

[Philip Rakity]

Doesn't the platform data hold the specific board specific quirks ?

Can you provide an example of how you see the platform data being used.  I think we are saying the same thing using different words but a code snippet would clarify this.

Philip

On Nov 9, 2010, at 3:55 AM, Wolfram Sang wrote:

> On Tue, Nov 09, 2010 at 03:45:15AM -0800, Philip Rakity wrote:
>> 
>> reposting since we are discussing removing quirks.   
>> 
>> We could partition the quirks into one set for controller quirks and one set for platform quirks.  That would give us another 32 quirks. 
> 
> IMHO we need less quirks, not more. Adding the number of potential
> quirks just delays the underlying problem. Also, unless someone
> enlightens me, I don't think there are platform quirks, only
> platform_data?
> 
> Regards,
> 
>   Wolfram
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: can we remove the quirk SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN ?

[Alan Cox]

I think I agree with Wolfram. Perhaps a much better model would be

"Anyone who adds a quirk will have to figure out how to remove one"

Several are trivial to remove and generalise into a couple of function
overrides.

Alan

Re: can we remove the quirk SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN ?

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

Hi Philip,

> Doesn't the platform data hold the specific board specific quirks ?

If we agree on not calling it "quirks" but "configuration", then we may
in deed be talking about the very same thing :)

> Can you provide an example of how you see the platform data being
> used.

Sad thing is that I don't have a board using an 8 bit width bus :(

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: can we remove the quirk SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN ?

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]

On Tue, Nov 09, 2010 at 12:08:35PM +0000, Alan Cox wrote:
> I think I agree with Wolfram. Perhaps a much better model would be
> 
> "Anyone who adds a quirk will have to figure out how to remove one"
> 
> Several are trivial to remove and generalise into a couple of function
> overrides.

My favourite would be one concentrated action to remove a couple of
those quirks while adding well defined hooks to reduce the need of new
ones. I have a few ideas and am currently trying a few options to get it
funded :)

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]