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

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:	Tue, 13 May 2014 10:19:44 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:	Russell King <linux@....linux.org.uk>,
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	Chris Ball <chris@...ntf.net>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	agross@...cinc.com,
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v1 08/11] mmc: mmci: Qcom fix MCICLK register settings.

On Tue, Apr 29, 2014 at 10:20 AM,  <srinivas.kandagatla@...aro.org> wrote:

> From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>
> 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@...aro.org>
(...)

> -       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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/