Re: [PATCH v1 08/11] mmc: mmci: Qcom fix MCICLK register settings.

[Linus Walleij]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/