On Tue, Apr 29, 2014 at 10:20 AM, <srinivas.kandagatla@xxxxxxxxxx> wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
>
> MCICLK register layout is bit different to the standard pl180 register layout.
> Qcom SDCC controller some setup in MCICLK register to get it going. So this
> patch adds new setup and makes it specific to Qcom hw designer.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
(...)
> - if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_4)
> - clk |= MCI_4BIT_BUS;
> - if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_8)
> - clk |= MCI_ST_8BIT_BUS;
> + if (host->hw_designer == AMBA_VENDOR_QCOM) {
> + clk |= MCI_CLK_QCOM_FLOWENA;
> + clk |= (MCI_CLK_QCOM_SEL_FEEDBACK_CLK <<
> + MCI_CLK_QCOM_SEL_IN_SHIFT); /* feedback clk */
> + if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_8)
> + clk |= MCI_CLK_QCOM_WIDEBUS_8;
> + else if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_4)
> + clk |= MCI_CLK_QCOM_WIDEBUS_4;
> + else
> + clk |= MCI_CLK_QCOM_WIDEBUS_1;
> +
> + if (host->mmc->ios.timing == MMC_TIMING_UHS_DDR50) {
> + /* clear SELECT_IN field */
> + clk &= ~(MCI_CLK_QCOM_SEL_MASK <<
> + MCI_CLK_QCOM_SEL_IN_SHIFT);
> + /* set DDR timing mode */
> + clk |= (MCI_CLK_QCOM_SEL_DDR_MODE <<
> + MCI_CLK_QCOM_SEL_IN_SHIFT);
> + }
> + clk |= (MCI_CLK_SDC4_MCLK_SEL_MCLK <<
> + MCI_CLK_SDC4_MCLK_SEL_SHIFT);
>
> - if (host->mmc->ios.timing == MMC_TIMING_UHS_DDR50)
> - clk |= MCI_ST_UX500_NEG_EDGE;
> + } else {
> + if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_4)
> + clk |= MCI_4BIT_BUS;
> + if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_8)
> + clk |= MCI_ST_8BIT_BUS;
> +
> + if (host->mmc->ios.timing == MMC_TIMING_UHS_DDR50)
> + clk |= MCI_ST_UX500_NEG_EDGE;
> + }
Again follow the pattern of storing register templates in the vendor_data
struct. I think you will quickly realize how this can be cut down with
new fields like .clk_4bitmode etc.
> /* Modified PL180 on Versatile Express platform */
> #define MCI_ARM_HWFCEN (1 << 12)
>
> +/* Modified on Qualcomm Integrations */
First: follow the convention set for the ST-specific registers that
look e.g. like this:
MCI_ST_8BIT_BUS i.e. MCI_<vendor>_SPECIFIER
So the below becomes MCI_QCOM_CLK_WIDEBUS... etc.
> +#define MCI_CLK_QCOM_WIDEBUS_1 (0 << 10)
> +#define MCI_CLK_QCOM_WIDEBUS_4 (2 << 10)
> +#define MCI_CLK_QCOM_WIDEBUS_8 (3 << 10)
Compare to what we have:
#define MMCICLOCK 0x004
#define MCI_CLK_ENABLE (1 << 8)
#define MCI_CLK_PWRSAVE (1 << 9)
#define MCI_CLK_BYPASS (1 << 10)
#define MCI_4BIT_BUS (1 << 11)
MCI_CLK_QCOM_WIDEBUS_1 is clearly surplus since it is
not setting any bit.
MCI_CLK_QCOM_WIDEBUS_4 (2 << 10) ==
MCI_4BIT_BUS (1 <<11), this is the same thing just
expressed in two ways! No need to have some surplus
definition adding to the confusion. Just use MCI_4BIT_BUS
The only thing that is really different is
MCI_CLK_QCOM_WIDEBUS_8 which whould have
the name (following the sibling definitions for the ST block):
#define MCI_QCOM_8BIT_BUS (3 << 10)
> +#define MCI_CLK_QCOM_FLOWENA (1 << 12)
Rename:
MCI_QCOM_CLK_HWFCEN (1<<13)
to match sibling definitions. It's clear that this enabled
hardware flow control and that name is more helpful to
understand what is going on.
> +#define MCI_CLK_QCOM_INVERTOUT (1 << 13)
Rename:
MCI_QCOM_CLK_INV
To match siblings.
> +/* select in latch data and command */
> +#define MCI_CLK_QCOM_SEL_IN_SHIFT (14)
> +#define MCI_CLK_QCOM_SEL_MASK (0x3)
> +#define MCI_CLK_QCOM_SEL_RISING_EDGE (1)
> +#define MCI_CLK_QCOM_SEL_FEEDBACK_CLK (2)
> +#define MCI_CLK_QCOM_SEL_DDR_MODE (3)
> +
> +/* mclk selection */
> +#define MCI_CLK_SDC4_MCLK_SEL_SHIFT (23)
> +#define MCI_CLK_SDC4_MCLK_SEL_MASK (0x3)
> +#define MCI_CLK_SDC4_MCLK_SEL_FB_CLK (1)
> +#define MCI_CLK_SDC4_MCLK_SEL_MCLK (2)
These seem to have no related siblings but should still be
renamed MCL_QCOM_*
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
