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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 26 May 2014 22:38:23 +0100
From:	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:	Ulf Hansson <ulf.hansson@...aro.org>
CC:	Russell King <linux@....linux.org.uk>,
	linux-mmc <linux-mmc@...r.kernel.org>,
	Chris Ball <chris@...ntf.net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-arm-msm@...r.kernel.org,
	Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [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@...aro.org> wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@...aro.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@...aro.org>
>> Reviewed-by: Linus Walleij <linus.walleij@...aro.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/