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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0baf8584-7833-0917-4432-363b0ee31bbc@codeaurora.org>
Date:   Thu, 24 May 2018 18:30:50 +0530
From:   Vijay Viswanath <vviswana@...eaurora.org>
To:     Evan Green <evgreen@...omium.org>
Cc:     adrian.hunter@...el.com, Ulf Hansson <ulf.hansson@...aro.org>,
        robh+dt@...nel.org, mark.rutland@....com,
        linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org,
        shawn.lin@...k-chips.com, linux-arm-msm@...r.kernel.org,
        georgi.djakov@...aro.org, devicetree@...r.kernel.org,
        asutoshd@...eaurora.org, stummala@...eaurora.org,
        venkatg@...eaurora.org, jeremymc@...hat.com,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        riteshh@...eaurora.org, vbadigan@...eaurora.org,
        Doug Anderson <dianders@...gle.com>, sayalil@...eaurora.org
Subject: Re: [PATCH V1 3/3] mmc: host: Register changes for sdcc V5



On 5/22/2018 11:42 PM, Evan Green wrote:
> Hi Vijay. Thanks for this patch.
> 
> On Thu, May 17, 2018 at 3:30 AM Vijay Viswanath <vviswana@...eaurora.org>
> wrote:
> 
>> From: Sayali Lokhande <sayalil@...eaurora.org>
> 
>> For SDCC version 5.0.0 and higher, new compatible string
>> "qcom,sdhci-msm-v5" is added.
> 
>> Based on the msm variant, pick the relevant variant data and
>> use it for register read/write to msm specific registers.
> 
>> Signed-off-by: Sayali Lokhande <sayalil@...eaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@...eaurora.org>
>> ---
>>    .../devicetree/bindings/mmc/sdhci-msm.txt          |   5 +-
>>    drivers/mmc/host/sdhci-msm.c                       | 344
> +++++++++++++--------
>>    2 files changed, 222 insertions(+), 127 deletions(-)
> 
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> index bfdcdc4..c2b7b2b 100644
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> @@ -4,7 +4,10 @@ This file documents differences between the core
> properties in mmc.txt
>>    and the properties used by the sdhci-msm driver.
> 
>>    Required properties:
>> -- compatible: Should contain "qcom,sdhci-msm-v4".
>> +- compatible: Should contain "qcom,sdhci-msm-v4" or "qcom,sdhci-msm-v5".
>> +                For SDCC version 5.0.0, MCI registers are removed from
> SDCC
>> +                interface and some registers are moved to HC. New
> compatible
>> +                string is added to support this change -
> "qcom,sdhci-msm-v5".
>>    - reg: Base address and length of the register in the following order:
>>           - Host controller register map (required)
>>           - SD Core register map (required)
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index bb2bb59..408e6b2 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -33,16 +33,11 @@
>>    #define CORE_MCI_GENERICS              0x70
>>    #define SWITCHABLE_SIGNALING_VOLTAGE   BIT(29)
> 
>> -#define CORE_HC_MODE           0x78
> 
> Remove CORE_MCI_VERSION as well.
> 

Missed it. Will remove

>>    #define HC_MODE_EN             0x1
>>    #define CORE_POWER             0x0
>>    #define CORE_SW_RST            BIT(7)
>>    #define FF_CLK_SW_RST_DIS      BIT(13)
> 
>> -#define CORE_PWRCTL_STATUS     0xdc
>> -#define CORE_PWRCTL_MASK       0xe0
>> -#define CORE_PWRCTL_CLEAR      0xe4
>> -#define CORE_PWRCTL_CTL                0xe8
>>    #define CORE_PWRCTL_BUS_OFF    BIT(0)
>>    #define CORE_PWRCTL_BUS_ON     BIT(1)
>>    #define CORE_PWRCTL_IO_LOW     BIT(2)
>> @@ -63,17 +58,13 @@
>>    #define CORE_CDR_EXT_EN                BIT(19)
>>    #define CORE_DLL_PDN           BIT(29)
>>    #define CORE_DLL_RST           BIT(30)
>> -#define CORE_DLL_CONFIG                0x100
>>    #define CORE_CMD_DAT_TRACK_SEL BIT(0)
>> -#define CORE_DLL_STATUS                0x108
> 
>> -#define CORE_DLL_CONFIG_2      0x1b4
>>    #define CORE_DDR_CAL_EN                BIT(0)
>>    #define CORE_FLL_CYCLE_CNT     BIT(18)
>>    #define CORE_DLL_CLOCK_DISABLE BIT(21)
> 
>> -#define CORE_VENDOR_SPEC       0x10c
>> -#define CORE_VENDOR_SPEC_POR_VAL       0xa1c
>> +#define CORE_VENDOR_SPEC_POR_VAL 0xa1c
>>    #define CORE_CLK_PWRSAVE       BIT(1)
>>    #define CORE_HC_MCLK_SEL_DFLT  (2 << 8)
>>    #define CORE_HC_MCLK_SEL_HS400 (3 << 8)
>> @@ -111,17 +102,14 @@
>>    #define CORE_CDC_SWITCH_BYPASS_OFF     BIT(0)
>>    #define CORE_CDC_SWITCH_RC_EN          BIT(1)
> 
>> -#define CORE_DDR_200_CFG               0x184
>>    #define CORE_CDC_T4_DLY_SEL            BIT(0)
>>    #define CORE_CMDIN_RCLK_EN             BIT(1)
>>    #define CORE_START_CDC_TRAFFIC         BIT(6)
>> -#define CORE_VENDOR_SPEC3      0x1b0
>> +
>>    #define CORE_PWRSAVE_DLL       BIT(3)
> 
>> -#define CORE_DDR_CONFIG                0x1b8
>>    #define DDR_CONFIG_POR_VAL     0x80040853
> 
>> -#define CORE_VENDOR_SPEC_CAPABILITIES0 0x11c
> 
>>    #define INVALID_TUNING_PHASE   -1
>>    #define SDHCI_MSM_MIN_CLOCK    400000
>> @@ -380,10 +368,14 @@ static inline int msm_dll_poll_ck_out_en(struct
> sdhci_host *host, u8 poll)
>>           u32 wait_cnt = 50;
>>           u8 ck_out_en;
>>           struct mmc_host *mmc = host->mmc;
>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +       const struct sdhci_msm_offset *msm_offset =
>> +                                       msm_host->offset;
> 
> I notice this pattern is pasted all over the place in order to get to the
> offsets. Maybe a macro or inlined function would be cleaner to get you to
> directly to the sdhci_msm_offset struct from sdhci_host, rather than this
> blob of paste soup everywhere. In some places you do seem to use the
> intermediate locals, so those cases wouldn't need to use the new helper.
> 
> 
>>           /* Poll for CK_OUT_EN bit.  max. poll time = 50us */
>> -       ck_out_en = !!(readl_relaxed(host->ioaddr + CORE_DLL_CONFIG) &
>> -                       CORE_CK_OUT_EN);
>> +       ck_out_en = !!(readl_relaxed(host->ioaddr +
>> +                       msm_offset->core_dll_config) & CORE_CK_OUT_EN);
> 
>>           while (ck_out_en != poll) {
>>                   if (--wait_cnt == 0) {
>> @@ -393,8 +385,8 @@ static inline int msm_dll_poll_ck_out_en(struct
> sdhci_host *host, u8 poll)
>>                   }
>>                   udelay(1);
> 
>> -               ck_out_en = !!(readl_relaxed(host->ioaddr +
> CORE_DLL_CONFIG) &
>> -                               CORE_CK_OUT_EN);
>> +               ck_out_en = !!(readl_relaxed(host->ioaddr +
>> +                       msm_offset->core_dll_config) & CORE_CK_OUT_EN);
>>           }
> 
>>           return 0;
>> @@ -410,16 +402,20 @@ static int msm_config_cm_dll_phase(struct
> sdhci_host *host, u8 phase)
>>           unsigned long flags;
>>           u32 config;
>>           struct mmc_host *mmc = host->mmc;
>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +       const struct sdhci_msm_offset *msm_offset =
>> +                                       msm_host->offset;
> 
>>           if (phase > 0xf)
>>                   return -EINVAL;
> 
>>           spin_lock_irqsave(&host->lock, flags);
> 
>> -       config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> +       config = readl_relaxed(host->ioaddr +
> msm_offset->core_dll_config);
>>           config &= ~(CORE_CDR_EN | CORE_CK_OUT_EN);
>>           config |= (CORE_CDR_EXT_EN | CORE_DLL_EN);
>> -       writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> +       writel_relaxed(config, host->ioaddr +
> msm_offset->core_dll_config);
> 
>>           /* Wait until CK_OUT_EN bit of DLL_CONFIG register becomes '0' */
>>           rc = msm_dll_poll_ck_out_en(host, 0);
>> @@ -430,24 +426,24 @@ static int msm_config_cm_dll_phase(struct
> sdhci_host *host, u8 phase)
>>            * Write the selected DLL clock output phase (0 ... 15)
>>            * to CDR_SELEXT bit field of DLL_CONFIG register.
>>            */
>> -       config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> +       config = readl_relaxed(host->ioaddr +
> msm_offset->core_dll_config);
>>           config &= ~CDR_SELEXT_MASK;
>>           config |= grey_coded_phase_table[phase] << CDR_SELEXT_SHIFT;
>> -       writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> +       writel_relaxed(config, host->ioaddr +
> msm_offset->core_dll_config);
> 
>> -       config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> +       config = readl_relaxed(host->ioaddr +
> msm_offset->core_dll_config);
>>           config |= CORE_CK_OUT_EN;
>> -       writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> +       writel_relaxed(config, host->ioaddr +
> msm_offset->core_dll_config);
> 
>>           /* Wait until CK_OUT_EN bit of DLL_CONFIG register becomes '1' */
>>           rc = msm_dll_poll_ck_out_en(host, 1);
>>           if (rc)
>>                   goto err_out;
> 
>> -       config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> +       config = readl_relaxed(host->ioaddr +
> msm_offset->core_dll_config);
>>           config |= CORE_CDR_EN;
>>           config &= ~CORE_CDR_EXT_EN;
>> -       writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> +       writel_relaxed(config, host->ioaddr +
> msm_offset->core_dll_config);
> 
> Nit: host->ioaddr + msm_offset->core_dll_config might benefit from having
> its own local, since you use it so much in this function. Same goes for
> where I've noted below...
> 

core_dll_config is very much used. But having a local for it feels like 
a bad idea. As different versions come up, the most used register may 
change. So it would be better to stick to a consistent approach to 
accessing every register.

>>           goto out;
> 
>>    err_out:
>> @@ -573,6 +569,10 @@ static int msm_find_most_appropriate_phase(struct
> sdhci_host *host,
>>    static inline void msm_cm_dll_set_freq(struct sdhci_host *host)
>>    {
>>           u32 mclk_freq = 0, config;
>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +       const struct sdhci_msm_offset *msm_offset =
>> +                                       msm_host->offset;
> 
>>           /* Program the MCLK value to MCLK_FREQ bit field */
>>           if (host->clock <= 112000000)
>> @@ -592,10 +592,10 @@ static inline void msm_cm_dll_set_freq(struct
> sdhci_host *host)
>>           else if (host->clock <= 200000000)
>>                   mclk_freq = 7;
> 
>> -       config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> +       config = readl_relaxed(host->ioaddr +
> msm_offset->core_dll_config);
>>           config &= ~CMUX_SHIFT_PHASE_MASK;
>>           config |= mclk_freq << CMUX_SHIFT_PHASE_SHIFT;
>> -       writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> +       writel_relaxed(config, host->ioaddr +
> msm_offset->core_dll_config);
>>    }
> 
>>    /* Initialize the DLL (Programmable Delay Line) */
>> @@ -607,6 +607,8 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>>           int wait_cnt = 50;
>>           unsigned long flags;
>>           u32 config;
>> +       const struct sdhci_msm_offset *msm_offset =
>> +                                       msm_host->offset;
> 
>>           spin_lock_irqsave(&host->lock, flags);
> 
>> @@ -615,34 +617,43 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>>            * tuning is in progress. Keeping PWRSAVE ON may
>>            * turn off the clock.
>>            */
>> -       config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC);
>> +       config = readl_relaxed(host->ioaddr +
> msm_offset->core_vendor_spec);
>>           config &= ~CORE_CLK_PWRSAVE;
>> -       writel_relaxed(config, host->ioaddr + CORE_VENDOR_SPEC);
>> +       writel_relaxed(config, host->ioaddr +
> msm_offset->core_vendor_spec);
> 
>>           if (msm_host->use_14lpp_dll_reset) {
>> -               config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> +               config = readl_relaxed(host->ioaddr +
>> +                               msm_offset->core_dll_config);
>>                   config &= ~CORE_CK_OUT_EN;
>> -               writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> +               writel_relaxed(config, host->ioaddr +
>> +                               msm_offset->core_dll_config);
> 
>> -               config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
>> +               config = readl_relaxed(host->ioaddr +
>> +                               msm_offset->core_dll_config_2);
>>                   config |= CORE_DLL_CLOCK_DISABLE;
>> -               writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
>> +               writel_relaxed(config, host->ioaddr +
>> +                               msm_offset->core_dll_config_2);
>>           }
> 
>> -       config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> +       config = readl_relaxed(host->ioaddr +
>> +                       msm_offset->core_dll_config);
>>           config |= CORE_DLL_RST;
>> -       writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> +       writel_relaxed(config, host->ioaddr +
>> +                       msm_offset->core_dll_config);
> 
>> -       config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> +       config = readl_relaxed(host->ioaddr +
>> +                       msm_offset->core_dll_config);
>>           config |= CORE_DLL_PDN;
>> -       writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> +       writel_relaxed(config, host->ioaddr +
>> +                       msm_offset->core_dll_config);
>>           msm_cm_dll_set_freq(host);
> 
>>           if (msm_host->use_14lpp_dll_reset &&
>>               !IS_ERR_OR_NULL(msm_host->xo_clk)) {
>>                   u32 mclk_freq = 0;
> 
>> -               config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
>> +               config = readl_relaxed(host->ioaddr +
>> +                               msm_offset->core_dll_config_2);
>>                   config &= CORE_FLL_CYCLE_CNT;
>>                   if (config)
>>                           mclk_freq = DIV_ROUND_CLOSEST_ULL((host->clock *
> 8),
>> @@ -651,40 +662,52 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>>                           mclk_freq = DIV_ROUND_CLOSEST_ULL((host->clock *
> 4),
>>                                           clk_get_rate(msm_host->xo_clk));
> 
>> -               config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
>> +               config = readl_relaxed(host->ioaddr +
>> +                               msm_offset->core_dll_config_2);
>>                   config &= ~(0xFF << 10);
>>                   config |= mclk_freq << 10;
> 
>> -               writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
>> +               writel_relaxed(config, host->ioaddr +
>> +                               msm_offset->core_dll_config_2);
>>                   /* wait for 5us before enabling DLL clock */
>>                   udelay(5);
>>           }
> 
>> -       config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> +       config = readl_relaxed(host->ioaddr +
>> +                       msm_offset->core_dll_config);
>>           config &= ~CORE_DLL_RST;
>> -       writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> +       writel_relaxed(config, host->ioaddr +
>> +                       msm_offset->core_dll_config);
> 
>> -       config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> +       config = readl_relaxed(host->ioaddr +
>> +                       msm_offset->core_dll_config);
>>           config &= ~CORE_DLL_PDN;
>> -       writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> +       writel_relaxed(config, host->ioaddr +
>> +                       msm_offset->core_dll_config);
> 
>>           if (msm_host->use_14lpp_dll_reset) {
>>                   msm_cm_dll_set_freq(host);
>> -               config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
>> +               config = readl_relaxed(host->ioaddr +
>> +                               msm_offset->core_dll_config_2);
>>                   config &= ~CORE_DLL_CLOCK_DISABLE;
>> -               writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
>> +               writel_relaxed(config, host->ioaddr +
>> +                               msm_offset->core_dll_config_2);
>>           }
> 
>> -       config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> +       config = readl_relaxed(host->ioaddr +
>> +                       msm_offset->core_dll_config);
>>           config |= CORE_DLL_EN;
>> -       writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> +       writel_relaxed(config, host->ioaddr +
>> +                       msm_offset->core_dll_config);
> 
>> -       config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> +       config = readl_relaxed(host->ioaddr +
>> +                       msm_offset->core_dll_config);
>>           config |= CORE_CK_OUT_EN;
>> -       writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> +       writel_relaxed(config, host->ioaddr +
>> +                       msm_offset->core_dll_config);
> 
> 
> ...here. A local for host->ioaddr + msm_offset->core_dll_config would save
> you a lot of split lines.
> 
>> @@ -1272,12 +1327,17 @@ static void sdhci_msm_dump_pwr_ctrl_regs(struct
> sdhci_host *host)
>>    {
>>           struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>           struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +       const struct sdhci_msm_offset *msm_offset =
>> +                                       msm_host->offset;
> 
>>           pr_err("%s: PWRCTL_STATUS: 0x%08x | PWRCTL_MASK: 0x%08x |
> PWRCTL_CTL: 0x%08x\n",
>> -                       mmc_hostname(host->mmc),
>> -                       readl_relaxed(msm_host->core_mem +
> CORE_PWRCTL_STATUS),
>> -                       readl_relaxed(msm_host->core_mem +
> CORE_PWRCTL_MASK),
>> -                       readl_relaxed(msm_host->core_mem +
> CORE_PWRCTL_CTL));
>> +               mmc_hostname(host->mmc),
>> +                msm_host->var_ops->msm_readl_relaxed(host,
> 
> There's a weird extra space here.
> 
>> +                       msm_offset->core_pwrctl_status),
>> +               msm_host->var_ops->msm_readl_relaxed(host,
>> +                       msm_offset->core_pwrctl_mask),
>> +               msm_host->var_ops->msm_readl_relaxed(host,
>> +                       msm_offset->core_pwrctl_ctl));
> 
> I think the idea of function pointers is fine, but overall the use of them
> everywhere sure is ugly. It makes it really hard to actually see what's
> happening. I wonder if things might look a lot cleaner with a helper
> function here. Then instead of:
> 
> msm_host->var_ops->msm_readl_relaxed(host, msm_offset->core_pwrctl_ctl);
> 
> You could have
> 
> msm_core_read(host, msm_offset->core_pwrctl_ctl);
> 

if we use a helper function, then we will have to pass msm_host into it 
as well. Otherwise there would be the hassle of deriving msm_host 
address from sdhci_host.

How about using a MACRO here instead for readability ?

>> @@ -1553,7 +1619,8 @@ static void sdhci_msm_set_regulator_caps(struct
> sdhci_msm_host *msm_host)
>>                    */
>>                   u32 io_level = msm_host->curr_io_level;
> 
>> -               config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC);
>> +               config =  readl_relaxed(host->ioaddr +
>> +                               msm_offset->core_vendor_spec);
> 
> Remove the second space before the readl_relaxed.
> 
>> @@ -1710,32 +1793,40 @@ static int sdhci_msm_probe(struct platform_device
> *pdev)
>>                   dev_warn(&pdev->dev, "TCXO clk not present (%d)\n", ret);
>>           }
> 
>> -       core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> -       msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
> core_memres);
>> +       if (!msm_host->mci_removed) {
>> +               core_memres = platform_get_resource(pdev, IORESOURCE_MEM,
> 1);
>> +               msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
>> +                               core_memres);
> 
>> -       if (IS_ERR(msm_host->core_mem)) {
>> -               dev_err(&pdev->dev, "Failed to remap registers\n");
>> -               ret = PTR_ERR(msm_host->core_mem);
>> -               goto clk_disable;
>> +               if (IS_ERR(msm_host->core_mem)) {
>> +                       dev_err(&pdev->dev, "Failed to remap
> registers\n");
>> +                       ret = PTR_ERR(msm_host->core_mem);
>> +                       goto clk_disable;
>> +               }
>>           }
> 
>>           /* Reset the vendor spec register to power on reset state */
>>           writel_relaxed(CORE_VENDOR_SPEC_POR_VAL,
>> -                      host->ioaddr + CORE_VENDOR_SPEC);
>> -
>> -       /* Set HC_MODE_EN bit in HC_MODE register */
>> -       writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
>> -
>> -       config = readl_relaxed(msm_host->core_mem + CORE_HC_MODE);
>> -       config |= FF_CLK_SW_RST_DIS;
>> -       writel_relaxed(config, msm_host->core_mem + CORE_HC_MODE);
>> +                       host->ioaddr + msm_offset->core_vendor_spec);
>> +
>> +       if (!msm_host->mci_removed) {
>> +               /* Set HC_MODE_EN bit in HC_MODE register */
>> +               msm_host->var_ops->msm_writel_relaxed(HC_MODE_EN, host,
>> +                               msm_offset->core_hc_mode);
>> +               config = msm_host->var_ops->msm_readl_relaxed(host,
>> +                               msm_offset->core_hc_mode);
>> +               config |= FF_CLK_SW_RST_DIS;
>> +               msm_host->var_ops->msm_writel_relaxed(config, host,
>> +                               msm_offset->core_hc_mode);
>> +       }
> 
>>           host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
>>           dev_dbg(&pdev->dev, "Host Version: 0x%x Vendor Version 0x%x\n",
>>                   host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
>>                                  SDHCI_VENDOR_VER_SHIFT));
> 
>> -       core_version = readl_relaxed(msm_host->core_mem +
> CORE_MCI_VERSION);
>> +       core_version =  msm_host->var_ops->msm_readl_relaxed(host,
>> +               msm_offset->core_mci_version);
> 
> Another double space after the =. Perhaps this was a find/replace error?
> Look out for more of these that I missed.
> 
> -Evan
> 

Thanks for pointing these out. Will check for such issues in all 3 patches.

Thanks,
Vijay

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ