Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753916AbbDHKjC (ORCPT ); Wed, 8 Apr 2015 06:39:02 -0400 Received: from mail-qk0-f180.google.com ([209.85.220.180]:32778 "EHLO mail-qk0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753870AbbDHKi5 (ORCPT ); Wed, 8 Apr 2015 06:38:57 -0400 MIME-Version: 1.0 In-Reply-To: References: <1426562035-16709-1-git-send-email-chaotian.jing@mediatek.com> <1426562035-16709-3-git-send-email-chaotian.jing@mediatek.com> Date: Wed, 8 Apr 2015 12:38:56 +0200 Message-ID: Subject: =?UTF-8?B?UmU6IOetlOWkjTogW1BBVENIIHYyIDIvNV0gbW1jOiBtZWRpYXRlazogQWRkIE1lZGlhdA==?= =?UTF-8?B?ZWsgTU1DIGRyaXZlcg==?= From: Ulf Hansson To: =?UTF-8?B?Q2hhb3RpYW4gSmluZyAo5LqV5pyd5aSpKQ==?= Cc: Rob Herring , Matthias Brugger , Chris Ball , Mark Rutland , =?UTF-8?B?SmFtZXNKSiBMaWFvICjlu5blu7rmmbop?= , srv_heupstream , Arnd Bergmann , "devicetree@vger.kernel.org" , HONGZHOU Yang , Catalin Marinas , linux-mmc , Will Deacon , "linux-kernel@vger.kernel.org" , "linux-gpio@vger.kernel.org" , Sascha Hauer , =?UTF-8?B?WWluZ2pvZSBDaGVuICjpmbPoi7HmtLIp?= , =?UTF-8?B?RWRkaWUgSHVhbmcgKOm7g+aZuuWCkSk=?= , =?UTF-8?B?QmluIFpoYW5nICjnq6Dmlowp?= , "linux-arm-kernel@lists.infradead.org" , "linux-mediatek@lists.infradead.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7288 Lines: 209 [...] >> > + >> > +struct mt_bdma_desc { >> > + u32 first_u32; >> > +#define BDMA_DESC_EOL (1 << 0) >> > +#define BDMA_DESC_CHECKSUM (0xff << 8) /* bit8 ~ bit15 */ >> > +#define BDMA_DESC_BLKPAD (1 << 17) >> > +#define BDMA_DESC_DWPAD (1 << 18) >> > + u32 next; >> > + u32 ptr; >> > + u32 second_u32; >> > +#define BDMA_DESC_BUFLEN (0xffff) /* bit0 ~ bit15 */ >> > +}; >> > + >> > +struct msdc_dma { >> > + struct scatterlist *sg; /* I/O scatter list */ >> > + struct mt_gpdma_desc *gpd; /* pointer to gpd >> array */ >> > + struct mt_bdma_desc *bd; /* pointer to bd >> array */ >> > + dma_addr_t gpd_addr; /* the physical address of gpd array */ >> > + dma_addr_t bd_addr; /* the physical address of bd array */ >> > +}; >> >> This looks weird from DMA perspective. Can you elaborate on why you >> can't use the dmaengine API? >> > The gpd and bd structure are determined by the MSDC hw, and, the DMA controller is a part of the MSDC hw, different with the chain dma implemented by the kernel. Hmm. I haven't reviewed the DMA related parts in detail. I will do that when you have sent the next version. >> > + >> > +struct msdc_host { >> > + struct device *dev; >> > + struct mmc_host *mmc; /* mmc structure */ >> > + int cmd_rsp; >> > + >> > + spinlock_t lock; >> > + struct mmc_request *mrq; >> > + struct mmc_command *cmd; >> > + struct mmc_data *data; >> > + int error; >> > + >> > + void __iomem *base; /* host base address */ >> > + >> > + struct msdc_dma dma; /* dma channel */ >> > + >> > + u32 timeout_ns; /* data timeout ns */ >> > + u32 timeout_clks; /* data timeout clks */ >> > + >> > + struct pinctrl *pinctrl; >> > + struct pinctrl_state *pins_default; >> > + struct pinctrl_state *pins_uhs; >> > + struct delayed_work req_timeout; >> > + int irq; /* host interrupt */ >> > + >> > + struct clk *src_clk; /* msdc source clock */ >> > + u32 mclk; /* mmc subsystem clock */ >> > + u32 hclk; /* host clock speed */ >> > + u32 sclk; /* SD/MS clock speed */ >> > + bool ddr; >> > +}; >> > + >> > +static void sdr_set_bits(void __iomem *reg, u32 bs) >> > +{ >> > + u32 val = readl(reg); >> > + >> > + val |= bs; >> > + writel(val, reg); >> > +} >> > + >> > +static void sdr_clr_bits(void __iomem *reg, u32 bs) >> > +{ >> > + u32 val = readl(reg); >> > + >> > + val &= ~(u32)bs; >> > + writel(val, reg); >> > +} >> > + >> > +static void sdr_set_field(void __iomem *reg, u32 field, u32 val) >> > +{ >> > + unsigned int tv = readl(reg); >> > + >> > + tv &= ~field; >> > + tv |= ((val) << (ffs((unsigned int)field) - 1)); >> > + writel(tv, reg); >> > +} >> >> A common thought for all the three above functions: >> >> Have you considered using a cache variable for those registers that >> often gets updated? In that way you would have to read the register >> value every time when you want to write to it. It should improve >> performance a bit. >> > These register can be modified by the MSDC hw, cannot cache it. Is that true for all registers? Anyway, let's leave this as is. >> > + >> > +static void sdr_get_field(void __iomem *reg, u32 field, u32 *val) >> > +{ >> > + unsigned int tv = readl(reg); >> > + >> > + *val = ((tv & field) >> (ffs((unsigned int)field) - 1)); >> > +} >> > + >> > +static void msdc_reset_hw(struct msdc_host *host) >> > +{ >> > + u32 val; >> > + >> > + sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_RST); >> > + while (readl(host->base + MSDC_CFG) & MSDC_CFG_RST) >> > + cpu_relax(); >> > + >> > + sdr_set_bits(host->base + MSDC_FIFOCS, MSDC_FIFOCS_CLR); >> > + while (readl(host->base + MSDC_FIFOCS) & MSDC_FIFOCS_CLR) >> > + cpu_relax(); >> > + >> > + val = readl(host->base + MSDC_INT); >> > + writel(val, host->base + MSDC_INT); >> > +} >> > + >> > +static void msdc_cmd_next(struct msdc_host *host, >> > + struct mmc_request *mrq, struct mmc_command *cmd); >> >> Please investigate whether you can move around the code to prevent >> this static declaration of the function. >> > It is hard to do it, because the first cmd may be CMD23, and do the next is send the mrq->cmd, so, there is a loop here, at least on method need do static declaration, Okay. [...] >> > +/* clock control primitives */ >> > +static void msdc_set_timeout(struct msdc_host *host, u32 ns, u32 clks) >> > +{ >> > + u32 timeout, clk_ns; >> > + u32 mode = 0; >> > + >> > + host->timeout_ns = ns; >> > + host->timeout_clks = clks; >> > + if (host->sclk == 0) { >> > + timeout = 0; >> > + } else { >> > + clk_ns = 1000000000UL / host->sclk; >> > + timeout = (ns + clk_ns - 1) / clk_ns + clks; >> > + /* in 1048576 sclk cycle unit */ >> > + timeout = (timeout + (1 << 20) - 1) >> 20; >> > + sdr_get_field(host->base + MSDC_CFG, >> MSDC_CFG_CKMOD, &mode); >> > + /*DDR mode will double the clk cycles for data timeout */ >> >> How do you know you will be using DDR at this point? Don't you need to >> check for that? >> > The MSDC_CFG_CKMODE can show current mode(DDR or SDR). Ah, thanks. Got it. [...] >> > +static void msdc_request_done(struct msdc_host *host, struct >> mmc_request *mrq) >> > +{ >> > + unsigned long flags; >> > + >> > + spin_lock_irqsave(&host->lock, flags); >> > + cancel_delayed_work(&host->req_timeout); >> >> This looks racy. >> >> Don't you need a cancel_delayed_work_sync() from somewhere, to be sure >> that work isn't preemted after the "host->mrq" has been reset here? Or >> maybe there is another way!? Of course that can't be done with the >> spin_locks held, but I asume you get my point. > Yes, I get your point, actually, we set a 5s timeout for each request, and, if the work already pending(means that some error happens), we do not wait it, > And the work will set the event to timeout and return, > So, maybe a good solution is that need check the return value of the cancel_delayed_work(), if it is pending, directly return, and do not set the host->mrq to 0 to avoid the race condition, That should work. [...] >> >> "ocr_avail" should be fetched from the vmmc regulator, when you invoke >> mmc_regulator_get_supply() above. > Yes, I also want to remove it, but, Not all devices use regulator, our SDIO device which connected on MSDC3, it's power is controlled by gpio. > This because the regulator of the PMU is not enough. So could you perhaps use a "gpio regulator" for this GPIO pin? There is already support to easily describe such in DT. Or it is a "reset GPIO" you are talking about? [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/