From patchwork Fri Oct 18 09:07:14 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Zhangfei Gao X-Patchwork-Id: 3065391 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 586D6BF924 for ; Fri, 18 Oct 2013 09:20:26 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 9F725204E2 for ; Fri, 18 Oct 2013 09:20:24 +0000 (UTC) Received: from casper.infradead.org (casper.infradead.org [85.118.1.10]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 90CFC20480 for ; Fri, 18 Oct 2013 09:20:22 +0000 (UTC) Received: from merlin.infradead.org ([2001:4978:20e::2]) by casper.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1VX62Y-0008JI-CZ; Fri, 18 Oct 2013 09:08:15 +0000 Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1VX621-0004xf-56; Fri, 18 Oct 2013 09:07:41 +0000 Received: from mail-ee0-x234.google.com ([2a00:1450:4013:c00::234]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1VX61x-0004xB-Jt for linux-arm-kernel@lists.infradead.org; Fri, 18 Oct 2013 09:07:39 +0000 Received: by mail-ee0-f52.google.com with SMTP id c41so1760346eek.25 for ; Fri, 18 Oct 2013 02:07:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=If8XYdaBTZuEuM2S6BPEeAtbKlRaPy0BEjRiNtSM3Uk=; b=BKIouoRVJHzGOeBpfjsyQE86M063tNd+h1L0gsVnJmL5AZTpyg9KmrOCcX+JHSD/GN OJaQZ1fk475VvzTdvhxmDS9IW6Yc6mIRxN43O29MhG9l8pKooROMaLs6HhHJ6hbL3Xaw r8VTa++tbD9UtB84IHpPQWsFTKAX14SFT/69SSwmTjvMjdbkSYo6r08A7pnP6uiQSoPK KRQ6rSEixvg9w0qQz83kXbobYgPMeE7Jkp+Ig9M19eV9HRXasKwzNjRAvQxVUcr5jIxy Gf6B9/iylja+cNowNYY+Ei4z9rGQMzt0Xf6MpnwE1bg6AeoNV4IEuYxtT1gvuQQdyowP Z1Bg== MIME-Version: 1.0 X-Received: by 10.14.3.9 with SMTP id 9mr1289760eeg.72.1382087234552; Fri, 18 Oct 2013 02:07:14 -0700 (PDT) Received: by 10.15.42.196 with HTTP; Fri, 18 Oct 2013 02:07:14 -0700 (PDT) In-Reply-To: <525BB90B.9060105@samsung.com> References: <1381415813-9395-1-git-send-email-zhangfei.gao@linaro.org> <1381415813-9395-3-git-send-email-zhangfei.gao@linaro.org> <525BB90B.9060105@samsung.com> Date: Fri, 18 Oct 2013 17:07:14 +0800 Message-ID: Subject: Re: [PATCH 2/2] mmc: add dw-mmc-k3 From: zhangfei gao To: Jaehoon Chung X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20131018_050737_964574_5CAB3CE3 X-CRM114-Status: GOOD ( 38.20 ) X-Spam-Score: -2.0 (--) Cc: Ulf Hansson , device-tree , "linux-mmc@vger.kernel.org" , Zhangfei Gao , Chris Ball , linux-arm-kernel X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Dear Jaehoon Thanks for the valueable suggestion. On Mon, Oct 14, 2013 at 5:27 PM, Jaehoon Chung wrote: > Hi, > > If you will send the patch, plz, add the prefix "mmc: dw_mmc: xxx" at subject. OK, got it. > And i think good that device-tree and mmc patch is separated. It is Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt, instead of dts. Should be combined with initial version? >> +* Hisilicon specific extensions to the Synopsis Designware Mobile >> + Storage Host Controller > Maybe fixed the Synopsys instead of Synopsis. OK, thanks >> +struct dw_mci_k3_priv_data { >> + enum dw_mci_k3_type type; >> + int old_timing; >> + u32 id; > You're using "id" as the "int" type at the below code. which type is exactly? >> + u32 gpio_cd; >> + u32 clken_reg; >> + u32 clken_bit; >> + u32 sam_sel_reg; >> + u32 sam_sel_bit; >> + u32 drv_sel_reg; >> + u32 drv_sel_bit; >> + u32 div_reg; >> + u32 div_bit; >> +}; > clken_reg/sam_sel_reg/drv_sel_reg can be changed? Otherwise, it's not host register? > If it's host register, then you can use the mci_writeX(). clken_reg/sam_sel_reg/drv_sel_reg is not host register, But other register from pctrl register, used for tuning clock, which is silicon special requirement. >> + >> +static void __iomem *pctrl; >> +static DEFINE_SPINLOCK(mmc_tuning_lock); >> +static int k3_tuning_config[][8][6] = { >> + /* bus_clk, div, drv_sel, sam_sel_max, sam_sel_min, input_clk */ >> + { >> + {180000000, 6, 6, 13, 13, 25000000}, /* 0: LEGACY 400k */ >> + {0}, /* 1: MMC_HS */ >> + {360000000, 6, 4, 2, 0, 50000000 }, /* 2: SD_HS */ >> + {180000000, 6, 4, 13, 13, 25000000}, /* 3: SDR12 */ >> + {360000000, 6, 4, 2, 0, 50000000 }, /* 4: SDR25 */ >> + {720000000, 6, 1, 9, 4, 100000000 }, /* 5: SDR50 */ >> + {0}, /* 6: SDR104 */ >> + {360000000, 7, 1, 3, 0, 50000000 }, /* 7: DDR50 */ >> + }, { >> + {26000000, 1, 1, 3, 3, 13000000 }, /* 0: LEGACY 400k */ >> + {360000000, 6, 3, 3, 1, 50000000 }, /* 1: MMC_HS*/ >> + {0}, /* 2: SD_HS */ >> + {0}, /* 3: SDR12 */ >> + {26000000, 1, 1, 3, 3, 13000000 }, /* 4: SDR25 */ >> + {360000000, 6, 3, 3, 1, 50000000 }, /* 5: SDR50 */ >> + {0}, /* 6: SDR104 */ >> + {720000000, 6, 4, 8, 4, 100000000}, /* 7: DDR50 */ >> + }, >> +}; > Can't this config be included the dt-file? Yes, good suggestion, it can be. However, additional API of_property_read_u32_array_index required. since there is no existing API get one array from list of arrays. Have test this API, may send out for comments, however, not sure it can ne accepted. What do you think? + * of_property_read_u32_array_index - Find and read an array of 32 bit integers + * pointed by index from a property. + * + * @np: device node from which the property value is to be read. + * @propname: name of the property to be searched. + * @out_values: pointer to return value, modified only if return value is 0. + * @sz: number of array elements to read + * @index: index of the u32 array in the list of values + * + * Search for a property in a device node and read 32-bit value(s) from + * it. Returns 0 on success, -EINVAL if the property does not exist, + * -ENODATA if property does not have a value, and -EOVERFLOW if the + * property data isn't large enough. + * + * The out_values is modified only if a valid u32 value can be decoded. + */ +int of_property_read_u32_array_index(const struct device_node *np, + const char *propname, u32 *out_values, + size_t sz, u32 index) +{ + const __be32 *val = of_find_property_value_of_size(np, propname, + (index + 1) * (sz * sizeof(*out_values))); + + if (IS_ERR(val)) + return PTR_ERR(val); + + while (index--) + val += sz; + while (sz--) + *out_values++ = be32_to_cpup(val++); + return 0; +} +EXPORT_SYMBOL_GPL(of_property_read_u32_array_index); >> + >> +static void dw_mci_k3_set_timing(struct dw_mci_k3_priv_data *priv, >> + int idx, int sam, int drv, int div) > Where is "idx" used? Yes, it can be removed. > >> +{ >> + unsigned int clken_reg = priv->clken_reg; >> + unsigned int clken_bit = priv->clken_bit; >> + unsigned int sam_sel_reg = priv->sam_sel_reg; >> + unsigned int sam_sel_bit = priv->sam_sel_bit; >> + unsigned int drv_sel_reg = priv->drv_sel_reg; >> + unsigned int drv_sel_bit = priv->drv_sel_bit; >> + unsigned int div_reg = priv->div_reg; >> + unsigned int div_bit = priv->div_bit; > Can't use the priv->xxx? OK. >> + int i = 0; >> + unsigned int temp_reg; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&mmc_tuning_lock, flags); >> + >> + /* disable clock */ >> + temp_reg = readl(pctrl + clken_reg); > temp_reg? is it register? OK, will use u32 val. >> + temp_reg &= ~(1<> + writel(temp_reg, pctrl + clken_reg); >> + >> + temp_reg = readl(pctrl + sam_sel_reg); >> + if (sam >= 0) { >> + /* set sam delay */ >> + for (i = 0; i < 4; i++) { >> + if (sam % 2) >> + temp_reg |= 1<<(sam_sel_bit + i); >> + else >> + temp_reg &= ~(1<<(sam_sel_bit + i)); >> + sam = sam >> 1; >> + } >> + } >> + writel(temp_reg, pctrl + sam_sel_reg); >> + >> + temp_reg = readl(pctrl + drv_sel_reg); >> + if (drv >= 0) { >> + /* set drv delay */ >> + for (i = 0; i < 4; i++) { >> + if (drv % 2) >> + temp_reg |= 1<<(drv_sel_bit + i); >> + else >> + temp_reg &= ~(1<<(drv_sel_bit + i)); >> + drv = drv >> 1; >> + } >> + } >> + writel(temp_reg, pctrl + drv_sel_reg); >> + >> + temp_reg = readl(pctrl + div_reg); >> + if (div >= 0) { >> + /* set drv delay */ >> + for (i = 0; i < 3; i++) { >> + if (div % 2) >> + temp_reg |= 1<<(div_bit + i); >> + else >> + temp_reg &= ~(1<<(div_bit + i)); >> + div = div >> 1; >> + } >> + } >> + writel(temp_reg, pctrl + div_reg); > I think these loop can reuse..like dw_mci_k3_set_delay(). It's duplicated. Yes, will update with function. >> + >> + /* enable clock */ >> + temp_reg = readl(pctrl + clken_reg); >> + temp_reg |= 1<> + writel(temp_reg, pctrl + clken_reg); >> + >> + spin_unlock_irqrestore(&mmc_tuning_lock, flags); >> +} >> + >> +static void dw_mci_k3_tun(struct dw_mci *host, int id, int index) >> +{ >> + struct dw_mci_k3_priv_data *priv = host->priv; >> + int ret; >> + >> + if (!pctrl) >> + return; >> + >> + if (priv->old_timing == index) > index? "timing" is more readable? OK. >> + return; >> + >> + ret = clk_set_rate(host->ciu_clk, k3_tuning_config[id][index][0]); >> + if (ret) >> + dev_err(host->dev, "clk_set_rate failed\n"); > not need to control about error? Just print the log? Will return then, mmc init will fail if clock set fail, so just pirnt error. >> + >> + dw_mci_k3_set_timing(priv, id, >> + (k3_tuning_config[id][index][3] + >> + k3_tuning_config[id][index][4]) / 2, >> + k3_tuning_config[id][index][2], >> + k3_tuning_config[id][index][1]); >> + >> + host->bus_hz = k3_tuning_config[id][index][5]; >> + priv->old_timing = index; >> +} >> + >> +static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios) >> +{ >> + struct dw_mci_k3_priv_data *priv = host->priv; >> + int id = priv->id; >> + >> + if (priv->type == DW_MCI_TYPE_HI4511) >> + dw_mci_k3_tun(host, id, ios->timing); > This code is just called the dw_mci_k3_tun()? > Then codes of dw_mci_k3_tun() can place into here? The dw_mci_k3_tun required at several places besides set_ios for example, after resume back, before init, otherwise the CTRL register fail to read on SD. >> +static int dw_mci_k3_setup_clock(struct dw_mci *host) >> +{ >> + struct dw_mci_k3_priv_data *priv = host->priv; >> + >> + if (priv->type == DW_MCI_TYPE_HI4511) >> + dw_mci_k3_tun(host, priv->id, MMC_TIMING_LEGACY); >> + >> + return 0; >> +} >> + >> + > remove the white space OK. >> +int dw_mci_k3_probe(struct platform_device *pdev) > static? thanks for point. >> +{ >> + const struct dw_mci_drv_data *drv_data; >> + const struct of_device_id *match; >> + struct dw_mci *host; >> + int gpio, err; >> + >> + match = of_match_node(dw_mci_k3_match, pdev->dev.of_node); >> + drv_data = match->data; >> + >> + err = dw_mci_pltfm_register(pdev, drv_data); >> + if (err) >> + return err; >> + >> + host = platform_get_drvdata(pdev); >> + if (host->pdata->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION) > Why return at here, when Broken card detection is set? >> + return 0; >> + >> + gpio = of_get_named_gpio(pdev->dev.of_node, "cd-gpio", 0); >> + if (gpio_is_valid(gpio)) { >> + if (devm_gpio_request(host->dev, gpio, "dw-mci-cd")) { >> + dev_err(host->dev, "gpio [%d] request failed\n", gpio); >> + } else { >> + struct dw_mci_k3_priv_data *priv = host->priv; >> + priv->gpio_cd = gpio; >> + host->pdata->get_cd = dw_mci_k3_get_cd; >> + err = devm_request_irq(host->dev, gpio_to_irq(gpio), >> + dw_mci_k3_card_detect, >> + IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING, >> + DRIVER_NAME, host); >> + if (err) >> + dev_warn(mmc_dev(host->dev), "request gpio irq error\n"); >> + } >> + >> + } else { >> + dev_info(host->dev, "cd gpio not available"); >> + } > I think it can use the slot-gpio.c. Yes, good suggesiton. However, dw_mmc.c will be modified accordingly, Paste here, What's your suggestion, will send out for review. { @@ -1893,6 +1923,11 @@ static int dw_mci_of_get_wp_gpio(struct device *dev, u8 slot) { return -EINVAL; } +static int dw_mci_of_get_cd_gpio(struct device *dev, u8 slot, + struct mmc_host *mmc) +{ + return -EINVAL; +} #endif /* CONFIG_OF */ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) @@ -1996,6 +2031,7 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) clear_bit(DW_MMC_CARD_PRESENT, &slot->flags); slot->wp_gpio = dw_mci_of_get_wp_gpio(host->dev, slot->id); + dw_mci_of_get_cd_gpio(host->dev, slot->id, mmc); ret = mmc_add_host(mmc); if (ret) >> + return 0; >> +} >> + >> +static int dw_mci_k3_suspend(struct device *dev) >> +{ >> + int ret; >> + struct dw_mci *host = dev_get_drvdata(dev); >> + >> + ret = dw_mci_suspend(host); >> + if (ret) >> + return ret; >> + >> + return 0; > ret = dw_mci_suspend(host); then > Just return ret; Yes, return dw_mci_suspend(host); will be used >> +SIMPLE_DEV_PM_OPS(dw_mci_k3_pmops, dw_mci_k3_suspend, dw_mci_k3_resume); >> + >> +static struct platform_driver dw_mci_k3_pltfm_driver = { >> + .probe = dw_mci_k3_probe, >> + .remove = dw_mci_pltfm_remove, >> + .driver = { >> + .name = DRIVER_NAME, > Just use the "dwmmc_k3" at here. not use the DRIVER_NAME. OK Thanks Zhangfei diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 018f365..9fe7f6a 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -35,6 +35,7 @@ #include #include #include +#include #include "dw_mmc.h" @@ -872,12 +873,15 @@ static int dw_mci_get_cd(struct mmc_host *mmc) int present; struct dw_mci_slot *slot = mmc_priv(mmc); struct dw_mci_board *brd = slot->host->pdata; + int gpio_cd = !mmc_gpio_get_cd(mmc); /* Use platform get_cd function, else try onboard card detect */ if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION) present = 1; else if (brd->get_cd) present = !brd->get_cd(slot->id); + else if (!IS_ERR_VALUE(gpio_cd)) + present = !!gpio_cd; else present = (mci_readl(slot->host, CDETECT) & (1 << slot->id)) == 0 ? 1 : 0; @@ -1876,6 +1880,32 @@ static int dw_mci_of_get_wp_gpio(struct device *dev, u8 slot) return gpio; } + +/* find the cd gpio for a given slot; or -1 if none specified */ +static int dw_mci_of_get_cd_gpio(struct device *dev, u8 slot, + struct mmc_host *mmc) +{ + struct device_node *np = dw_mci_of_find_slot_node(dev, slot); + int gpio; + + if (!np) + return -EINVAL; + + gpio = of_get_named_gpio(np, "cd-gpios", 0); + + /* Having a missing entry is valid; return silently */ + if (!gpio_is_valid(gpio)) { + printk("not valid gpio\n"); + return -EINVAL; + } + + if (mmc_gpio_request_cd(mmc, gpio, 0)) { + dev_warn(dev, "gpio [%d] request failed\n", gpio); + return -EINVAL; + } + + return gpio; +} #else /* CONFIG_OF */ static int dw_mci_of_get_slot_quirks(struct device *dev, u8 slot)