From mboxrd@z Thu Jan 1 00:00:00 1970 From: Srinivas Kandagatla Subject: Re: [PATCH v3 10/13] mmc: mmci: add Qcom specifics of clk and datactrl registers. Date: Mon, 26 May 2014 22:38:23 +0100 Message-ID: <5383B44F.30608@linaro.org> References: <1400849362-7007-1-git-send-email-srinivas.kandagatla@linaro.org> <1400849556-7501-1-git-send-email-srinivas.kandagatla@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-mmc-owner@vger.kernel.org To: Ulf Hansson Cc: Russell King , linux-mmc , Chris Ball , "linux-kernel@vger.kernel.org" , linux-arm-msm@vger.kernel.org, Linus Walleij List-Id: linux-arm-msm@vger.kernel.org Hi Ulf, Thanks for the comments. On 26/05/14 14:05, Ulf Hansson wrote: > On 23 May 2014 14:52, wrote: >> From: Srinivas Kandagatla >> >> 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 >> Reviewed-by: Linus Walleij >> --- >> 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