Re: [PATCH v3 10/13] mmc: mmci: add Qcom specifics of clk and datactrl registers.

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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