diff mbox

[06/10] mmc: sdhci: Add get_clk_div callback support

Message ID 1470923574-411-2-git-send-email-riteshh@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Ritesh Harjani Aug. 11, 2016, 1:52 p.m. UTC 
Few controllers (like MSM) may have to override div
in certain cases. Hence provide a callback to get the
div value for their driver.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
---
 drivers/mmc/host/sdhci.c | 8 ++++++++
 drivers/mmc/host/sdhci.h | 1 +
 2 files changed, 9 insertions(+)

Comments

Jaehoon Chung Aug. 12, 2016, 1:34 a.m. UTC | #1 
Hi,

On 08/11/2016 10:52 PM, Ritesh Harjani wrote:
> Few controllers (like MSM) may have to override div
> in certain cases. Hence provide a callback to get the
> div value for their driver.
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci.c | 8 ++++++++
>  drivers/mmc/host/sdhci.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index cd65d47..cc3d6f2 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1318,6 +1318,14 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>  clock_set:
>  	if (real_div)
>  		*actual_clock = (host->max_clk * clk_mul) / real_div;
> +	/*
> +	 * Few controllers may have to override div
> +	 * here. Hence provide a callback to get the
> +	 * div value for them.
> +	 */
> +	if (host->ops->get_clk_div)
> +		div = host->ops->get_clk_div(host, div);

This is for only getting your div value. Few controllers?
Rather, use the existent callback function..It's better than adding new callback.

In your controller, add the your set_clock() callback. not get_clk_div.
(Well, Adrian might have other opinion.)

Best Regards,
Jaehoon Chung


> +
>  	clk |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
>  	clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
>  		<< SDHCI_DIVIDER_HI_SHIFT;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0411c9f..4701001 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -562,6 +562,7 @@ struct sdhci_ops {
>  					 struct mmc_card *card,
>  					 unsigned int max_dtr, int host_drv,
>  					 int card_drv, int *drv_type);
> +	int	(*get_clk_div)(struct sdhci_host *host, int div);
>  };
>  
>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> 

--
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
Ritesh Harjani Aug. 12, 2016, 2:19 a.m. UTC | #2 
Hi Jaehoon/Adrian,


On 8/12/2016 7:04 AM, Jaehoon Chung wrote:
> Hi,
>
> On 08/11/2016 10:52 PM, Ritesh Harjani wrote:
>> Few controllers (like MSM) may have to override div
>> in certain cases. Hence provide a callback to get the
>> div value for their driver.
>>
>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>> ---
>>  drivers/mmc/host/sdhci.c | 8 ++++++++
>>  drivers/mmc/host/sdhci.h | 1 +
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index cd65d47..cc3d6f2 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1318,6 +1318,14 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>>  clock_set:
>>  	if (real_div)
>>  		*actual_clock = (host->max_clk * clk_mul) / real_div;
>> +	/*
>> +	 * Few controllers may have to override div
>> +	 * here. Hence provide a callback to get the
>> +	 * div value for them.
>> +	 */
>> +	if (host->ops->get_clk_div)
>> +		div = host->ops->get_clk_div(host, div);
>
> This is for only getting your div value. Few controllers?
> Rather, use the existent callback function..It's better than adding new callback.

As of today sdhci-of-arasan is the only user of this quirk 
-SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN. I was hoping that with this 
callback, we may get away with this quirk if sdhci-of-arasan can have 
get_clk_div callback implemented in it's driver?

Since I was not sure on this, so I did not modify sdhci-of-arasan. Thoughts?

>
> In your controller, add the your set_clock() callback. not get_clk_div.
> (Well, Adrian might have other opinion.)
Alright, please let me know what would be the right approach.

>
> Best Regards,
> Jaehoon Chung
>
>
>> +
>>  	clk |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
>>  	clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
>>  		<< SDHCI_DIVIDER_HI_SHIFT;
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 0411c9f..4701001 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -562,6 +562,7 @@ struct sdhci_ops {
>>  					 struct mmc_card *card,
>>  					 unsigned int max_dtr, int host_drv,
>>  					 int card_drv, int *drv_type);
>> +	int	(*get_clk_div)(struct sdhci_host *host, int div);
>>  };
>>
>>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>>
>
--
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
Shawn Lin Aug. 12, 2016, 3:21 a.m. UTC | #3 
在 2016/8/12 10:19, Ritesh Harjani 写道:
> Hi Jaehoon/Adrian,
>
>
> On 8/12/2016 7:04 AM, Jaehoon Chung wrote:
>> Hi,
>>
>> On 08/11/2016 10:52 PM, Ritesh Harjani wrote:
>>> Few controllers (like MSM) may have to override div
>>> in certain cases. Hence provide a callback to get the
>>> div value for their driver.
>>>
>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>>> ---
>>>  drivers/mmc/host/sdhci.c | 8 ++++++++
>>>  drivers/mmc/host/sdhci.h | 1 +
>>>  2 files changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index cd65d47..cc3d6f2 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1318,6 +1318,14 @@ u16 sdhci_calc_clk(struct sdhci_host *host,
>>> unsigned int clock,
>>>  clock_set:
>>>      if (real_div)
>>>          *actual_clock = (host->max_clk * clk_mul) / real_div;
>>> +    /*
>>> +     * Few controllers may have to override div
>>> +     * here. Hence provide a callback to get the
>>> +     * div value for them.
>>> +     */
>>> +    if (host->ops->get_clk_div)
>>> +        div = host->ops->get_clk_div(host, div);
>>
>> This is for only getting your div value. Few controllers?
>> Rather, use the existent callback function..It's better than adding
>> new callback.
>
> As of today sdhci-of-arasan is the only user of this quirk
> -SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN. I was hoping that with this
> callback, we may get away with this quirk if sdhci-of-arasan can have
> get_clk_div callback implemented in it's driver?
>
> Since I was not sure on this, so I did not modify sdhci-of-arasan.
> Thoughts?

yup, I'm still using SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN in
sdhci-of-arasan now. If you are addressing this, please go ahead.

Per previous disscussion of sdhci, it is deprecated to add new quirks
or callback(?) into sdhci. It should be better to make it a library.
 From this view, you should overwrite the set_clock in your variant
driver.

I has a question here, (like MSM) may have to override div
in certain cases. What certain cases is? I just see you simply
return 0 there which means you want to bypass the clk?
If that is a trick of clk rate, we could use clk framework API
there to lower the input clk to make the calculation produce
zero div?


>
>>
>> In your controller, add the your set_clock() callback. not get_clk_div.
>> (Well, Adrian might have other opinion.)
> Alright, please let me know what would be the right approach.
>
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>
>>> +
>>>      clk |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
>>>      clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
>>>          << SDHCI_DIVIDER_HI_SHIFT;
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index 0411c9f..4701001 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -562,6 +562,7 @@ struct sdhci_ops {
>>>                       struct mmc_card *card,
>>>                       unsigned int max_dtr, int host_drv,
>>>                       int card_drv, int *drv_type);
>>> +    int    (*get_clk_div)(struct sdhci_host *host, int div);
>>>  };
>>>
>>>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>>>
>>
> --
> 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
>
Ritesh Harjani Aug. 12, 2016, 3:46 a.m. UTC | #4 
Hi Shawn,

On 8/12/2016 8:51 AM, Shawn Lin wrote:
> 在 2016/8/12 10:19, Ritesh Harjani 写道:
>> Hi Jaehoon/Adrian,
>>
>>
>> On 8/12/2016 7:04 AM, Jaehoon Chung wrote:
>>> Hi,
>>>
>>> On 08/11/2016 10:52 PM, Ritesh Harjani wrote:
>>>> Few controllers (like MSM) may have to override div
>>>> in certain cases. Hence provide a callback to get the
>>>> div value for their driver.
>>>>
>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>>>> ---
>>>>  drivers/mmc/host/sdhci.c | 8 ++++++++
>>>>  drivers/mmc/host/sdhci.h | 1 +
>>>>  2 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index cd65d47..cc3d6f2 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -1318,6 +1318,14 @@ u16 sdhci_calc_clk(struct sdhci_host *host,
>>>> unsigned int clock,
>>>>  clock_set:
>>>>      if (real_div)
>>>>          *actual_clock = (host->max_clk * clk_mul) / real_div;
>>>> +    /*
>>>> +     * Few controllers may have to override div
>>>> +     * here. Hence provide a callback to get the
>>>> +     * div value for them.
>>>> +     */
>>>> +    if (host->ops->get_clk_div)
>>>> +        div = host->ops->get_clk_div(host, div);
>>>
>>> This is for only getting your div value. Few controllers?
>>> Rather, use the existent callback function..It's better than adding
>>> new callback.
>>
>> As of today sdhci-of-arasan is the only user of this quirk
>> -SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN. I was hoping that with this
>> callback, we may get away with this quirk if sdhci-of-arasan can have
>> get_clk_div callback implemented in it's driver?
>>
>> Since I was not sure on this, so I did not modify sdhci-of-arasan.
>> Thoughts?
>
> yup, I'm still using SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN in
> sdhci-of-arasan now. If you are addressing this, please go ahead.
>
> Per previous disscussion of sdhci, it is deprecated to add new quirks
> or callback(?) into sdhci. It should be better to make it a library.
On callback, let Adrian answer this. It will be helpful if the link of 
this discussion(to make sdhci a library) can be shared pls :)
It may be helpful if we have some (Do's and Don'ts), it may help others 
as well while making sdhci changes.

> From this view, you should overwrite the set_clock in your variant
> driver.
>
> I has a question here, (like MSM) may have to override div
> in certain cases. What certain cases is? I just see you simply
> return 0 there which means you want to bypass the clk?

Actually the commit was written by keeping in mind that sdhci-of-arasan 
will also use this callback. Since I was not sure, I thought of dropping 
changes of sdhci-of-arasan but did not change the commit msg.
Sorry for the confusion.

> If that is a trick of clk rate, we could use clk framework API
> there to lower the input clk to make the calculation produce
> zero div?
>
>
>>
>>>
>>> In your controller, add the your set_clock() callback. not get_clk_div.
>>> (Well, Adrian might have other opinion.)
>> Alright, please let me know what would be the right approach.
>>
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>
>>>> +
>>>>      clk |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
>>>>      clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
>>>>          << SDHCI_DIVIDER_HI_SHIFT;
>>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>>> index 0411c9f..4701001 100644
>>>> --- a/drivers/mmc/host/sdhci.h
>>>> +++ b/drivers/mmc/host/sdhci.h
>>>> @@ -562,6 +562,7 @@ struct sdhci_ops {
>>>>                       struct mmc_card *card,
>>>>                       unsigned int max_dtr, int host_drv,
>>>>                       int card_drv, int *drv_type);
>>>> +    int    (*get_clk_div)(struct sdhci_host *host, int div);
>>>>  };
>>>>
>>>>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>>>>
>>>
>> --
>> 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
>>
>
>
--
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
Shawn Lin Aug. 12, 2016, 7:21 a.m. UTC | #5 
在 2016/8/12 11:46, Ritesh Harjani 写道:
> Hi Shawn,
>
> On 8/12/2016 8:51 AM, Shawn Lin wrote:
>> 在 2016/8/12 10:19, Ritesh Harjani 写道:
>>> Hi Jaehoon/Adrian,
>>>
>>>
>>> On 8/12/2016 7:04 AM, Jaehoon Chung wrote:
>>>> Hi,
>>>>
>>>> On 08/11/2016 10:52 PM, Ritesh Harjani wrote:
>>>>> Few controllers (like MSM) may have to override div
>>>>> in certain cases. Hence provide a callback to get the
>>>>> div value for their driver.
>>>>>
>>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>>>>> ---
>>>>>  drivers/mmc/host/sdhci.c | 8 ++++++++
>>>>>  drivers/mmc/host/sdhci.h | 1 +
>>>>>  2 files changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>>> index cd65d47..cc3d6f2 100644
>>>>> --- a/drivers/mmc/host/sdhci.c
>>>>> +++ b/drivers/mmc/host/sdhci.c
>>>>> @@ -1318,6 +1318,14 @@ u16 sdhci_calc_clk(struct sdhci_host *host,
>>>>> unsigned int clock,
>>>>>  clock_set:
>>>>>      if (real_div)
>>>>>          *actual_clock = (host->max_clk * clk_mul) / real_div;
>>>>> +    /*
>>>>> +     * Few controllers may have to override div
>>>>> +     * here. Hence provide a callback to get the
>>>>> +     * div value for them.
>>>>> +     */
>>>>> +    if (host->ops->get_clk_div)
>>>>> +        div = host->ops->get_clk_div(host, div);
>>>>
>>>> This is for only getting your div value. Few controllers?
>>>> Rather, use the existent callback function..It's better than adding
>>>> new callback.
>>>
>>> As of today sdhci-of-arasan is the only user of this quirk
>>> -SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN. I was hoping that with this
>>> callback, we may get away with this quirk if sdhci-of-arasan can have
>>> get_clk_div callback implemented in it's driver?
>>>
>>> Since I was not sure on this, so I did not modify sdhci-of-arasan.
>>> Thoughts?
>>
>> yup, I'm still using SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN in
>> sdhci-of-arasan now. If you are addressing this, please go ahead.
>>
>> Per previous disscussion of sdhci, it is deprecated to add new quirks
>> or callback(?) into sdhci. It should be better to make it a library.
> On callback, let Adrian answer this. It will be helpful if the link of
> this discussion(to make sdhci a library) can be shared pls :)
> It may be helpful if we have some (Do's and Don'ts), it may help others
> as well while making sdhci changes.

Well, I just find this thread https://lkml.org/lkml/2016/1/27/309,
but there are some before that which I can't find now.

>
>> From this view, you should overwrite the set_clock in your variant
>> driver.
>>
>> I has a question here, (like MSM) may have to override div
>> in certain cases. What certain cases is? I just see you simply
>> return 0 there which means you want to bypass the clk?
>
> Actually the commit was written by keeping in mind that sdhci-of-arasan
> will also use this callback. Since I was not sure, I thought of dropping
> changes of sdhci-of-arasan but did not change the commit msg.
> Sorry for the confusion.
>
>> If that is a trick of clk rate, we could use clk framework API
>> there to lower the input clk to make the calculation produce
>> zero div?
>>
>>
>>>
>>>>
>>>> In your controller, add the your set_clock() callback. not get_clk_div.
>>>> (Well, Adrian might have other opinion.)
>>> Alright, please let me know what would be the right approach.
>>>
>>>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>>
>>>>> +
>>>>>      clk |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
>>>>>      clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
>>>>>          << SDHCI_DIVIDER_HI_SHIFT;
>>>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>>>> index 0411c9f..4701001 100644
>>>>> --- a/drivers/mmc/host/sdhci.h
>>>>> +++ b/drivers/mmc/host/sdhci.h
>>>>> @@ -562,6 +562,7 @@ struct sdhci_ops {
>>>>>                       struct mmc_card *card,
>>>>>                       unsigned int max_dtr, int host_drv,
>>>>>                       int card_drv, int *drv_type);
>>>>> +    int    (*get_clk_div)(struct sdhci_host *host, int div);
>>>>>  };
>>>>>
>>>>>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>>>>>
>>>>
>>> --
>>> 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
>>>
>>
>>
> --
> 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.c b/drivers/mmc/host/sdhci.c
index cd65d47..cc3d6f2 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1318,6 +1318,14 @@  u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 clock_set:
 	if (real_div)
 		*actual_clock = (host->max_clk * clk_mul) / real_div;
+	/*
+	 * Few controllers may have to override div
+	 * here. Hence provide a callback to get the
+	 * div value for them.
+	 */
+	if (host->ops->get_clk_div)
+		div = host->ops->get_clk_div(host, div);
+
 	clk |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
 	clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
 		<< SDHCI_DIVIDER_HI_SHIFT;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0411c9f..4701001 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -562,6 +562,7 @@  struct sdhci_ops {
 					 struct mmc_card *card,
 					 unsigned int max_dtr, int host_drv,
 					 int card_drv, int *drv_type);
+	int	(*get_clk_div)(struct sdhci_host *host, int div);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS