lkml.org 
[lkml]   [2014]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 10/13] mmc: mmci: add Qcom specifics of clk and datactrl registers.
Hi Ulf,

Thanks for the comments.

On 26/05/14 14:05, Ulf Hansson wrote:
> On 23 May 2014 14:52, <srinivas.kandagatla@linaro.org> wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds specifics of clk and datactrl register on Qualcomm SD
>> Card controller. This patch also populates the Qcom variant data with
>> these new values specific to Qualcomm SD Card Controller.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> drivers/mmc/host/mmci.c | 4 ++++
>> drivers/mmc/host/mmci.h | 24 ++++++++++++++++++++++++
>> 2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 17e7f6a..6434f5b1 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -185,6 +185,10 @@ static struct variant_data variant_qcom = {
>> .fifosize = 16 * 4,
>> .fifohalfsize = 8 * 4,
>> .clkreg = MCI_CLK_ENABLE,
>> + .clkreg_enable = MCI_QCOM_CLK_FLOWENA |
>> + MCI_QCOM_CLK_FEEDBACK_CLK,
>
> Obviously I don't have the in-depth knowledge about the Qcom variant,
> but comparing the ST variant here made me think.
>
> Using the feeback clock internal logic in the ST variant, requires the
> corresponding feedback clock pin signal on the board, to be
> routed/connected. Typically we used this for SD cards, which involved
> using an external level shifter circuit.
>
> Is it correct to enable this bit for all cases, including eMMC?
>
You are correct, FBCLK should specific to the board, and I will try to
do something on the same lines as ST variant in next version.

> If it is board specific configurations, you should add a DT binding
> for it - like there are for the ST variant.
>
>> + .clkreg_8bit_bus_enable = MCI_QCOM_CLK_WIDEBUS_8,
>> + .datactrl_mask_ddrmode = MCI_QCOM_CLK_DDR_MODE,
>> .blksz_datactrl4 = true,
>> .datalength_bits = 24,
>> .blksz_datactrl4 = true,
>> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
>> index cd83ca3..1b93ae7 100644
>> --- a/drivers/mmc/host/mmci.h
>> +++ b/drivers/mmc/host/mmci.h
>> @@ -41,6 +41,22 @@
>> /* Modified PL180 on Versatile Express platform */
>> #define MCI_ARM_HWFCEN BIT(12)
>>
>> +/* Modified on Qualcomm Integrations */
>> +#define MCI_QCOM_CLK_WIDEBUS_4 (2 << 10)
>
> This is the same as BIT(11), please use MCI_4BIT_BUS instead.
>
This is not used in the code, I will clean it up as you suggested, just
to be more consistent.

>> +#define MCI_QCOM_CLK_WIDEBUS_8 (3 << 10)
>
> Since you converted to use the "BIT" macro a few patches ago, I
> suggest we should stick to it. How about something below:
>
> #define MCI_QCOM_CLK_WIDEBUS_8 BIT (BIT(10) | BIT(11))
>
Sounds good, I will fix all such instances in next version.

> Please adopt all defines added in this patch to use the BIT macro.
>
>> +#define MCI_QCOM_CLK_FLOWENA BIT(12)
>> +#define MCI_QCOM_CLK_INVERTOUT BIT(13)
>> +
>> +/* select in latch data and command */
>> +#define MCI_QCOM_CLK_SEL_IN_SHIFT (14)
>
> BIT (14)?
>
>> +#define MCI_QCOM_CLK_SEL_MASK (0x3)
>> +#define MCI_QCOM_CLK_SEL_RISING_EDGE (1)
>
> BIT(1)?
>
>> +#define MCI_QCOM_CLK_FEEDBACK_CLK (2 << 14)
>> +#define MCI_QCOM_CLK_DDR_MODE (3 << 14)
>> +
>> +/* mclk selection */
>> +#define MCI_QCOM_CLK_SEL_MCLK (2 << 23)
>
> Does this correspond to MCI_CLK_BYPASS? If so, we should maybe state
> this in a comment?
>
No, this is not same as MCI_CLK_BYPASS, its selection between
FBCLK/gated MCLK/freerunning MCLK.

Thanks,
srini


\
 
 \ /
  Last update: 2014-05-27 00:41    [W:0.089 / U:2.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site