| Message ID | 1400849556-7501-1-git-send-email-srinivas.kandagatla@linaro.org |
|---|---|
| State | New |
| Headers | show |
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? 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. > +#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)) 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? > + > #define MMCIARGUMENT 0x008 > #define MMCICOMMAND 0x00c > #define MCI_CPSM_RESPONSE BIT(6) > @@ -54,6 +70,14 @@ > #define MCI_ST_NIEN BIT(13) > #define MCI_ST_CE_ATACMD BIT(14) > > +/* Modified on Qualcomm Integrations */ > +#define MCI_QCOM_CSPM_DATCMD BIT(12) > +#define MCI_QCOM_CSPM_MCIABORT BIT(13) > +#define MCI_QCOM_CSPM_CCSENABLE BIT(14) > +#define MCI_QCOM_CSPM_CCSDISABLE BIT(15) > +#define MCI_QCOM_CSPM_AUTO_CMD19 BIT(16) > +#define MCI_QCOM_CSPM_AUTO_CMD21 BIT(21) > + > #define MMCIRESPCMD 0x010 > #define MMCIRESPONSE0 0x014 > #define MMCIRESPONSE1 0x018 > -- > 1.9.1 > Kind regards Ulf Hansson -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf, On 26/05/14 22:38, Srinivas Kandagatla wrote: >>> 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. I get lot of I/O errors when I remove this flag for test. I rechecked schematics and datasheet, the feedback clk that we refer here is the the feedback clk from CLK pad, there is no separate input pad for fbclk. So I think this is internally feedbacked clk. This selection is configuring bits to latch data and command coming in using feedback clock from CLK pad. I will make sure that the macro is named more appropriately to reflect the same. thanks, srini -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 May 2014 11:41, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > Hi Ulf, > > > On 26/05/14 22:38, Srinivas Kandagatla wrote: >>>> >>>> 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. > > I get lot of I/O errors when I remove this flag for test. Running eMMC I suppose? > > I rechecked schematics and datasheet, the feedback clk that we refer here is > the the feedback clk from CLK pad, there is no separate input pad for fbclk. > So I think this is internally feedbacked clk. > > This selection is configuring bits to latch data and command coming in using > feedback clock from CLK pad. Seems like it's strange to have this bit configurable then. I guess it would be hard to tell under what circumstances you don't want this bit set. Anyway, it's not a big deal to me - let's keep it as is for now. Kind regards Ulf Hansson > > I will make sure that the macro is named more appropriately to reflect the > same. > > thanks, > srini -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, + .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) +#define MCI_QCOM_CLK_WIDEBUS_8 (3 << 10) +#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) +#define MCI_QCOM_CLK_SEL_MASK (0x3) +#define MCI_QCOM_CLK_SEL_RISING_EDGE (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) + #define MMCIARGUMENT 0x008 #define MMCICOMMAND 0x00c #define MCI_CPSM_RESPONSE BIT(6) @@ -54,6 +70,14 @@ #define MCI_ST_NIEN BIT(13) #define MCI_ST_CE_ATACMD BIT(14) +/* Modified on Qualcomm Integrations */ +#define MCI_QCOM_CSPM_DATCMD BIT(12) +#define MCI_QCOM_CSPM_MCIABORT BIT(13) +#define MCI_QCOM_CSPM_CCSENABLE BIT(14) +#define MCI_QCOM_CSPM_CCSDISABLE BIT(15) +#define MCI_QCOM_CSPM_AUTO_CMD19 BIT(16) +#define MCI_QCOM_CSPM_AUTO_CMD21 BIT(21) + #define MMCIRESPCMD 0x010 #define MMCIRESPONSE0 0x014 #define MMCIRESPONSE1 0x018