答复: [PATCH v2 2/5] mmc: mediatek: Add Mediatek MMC driver

Dear Ulf,

Thanks!
please help to check my comment:

On Wed, 2015-04-08 at 12:38 +0200, Ulf Hansson wrote:

[...]

quoted

quoted

quoted

+
+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 */

quoted

+       struct mt_bdma_desc *bd;                /* pointer to bd

array */

quoted

+       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.

quoted

quoted

quoted

+
+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.

quoted

quoted

quoted

+
+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.

[...]

quoted

quoted

quoted

+/* 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);

quoted

+               /*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.

[...]

quoted

quoted

quoted

+static void msdc_request_done(struct msdc_host *host, struct

mmc_request *mrq)

quoted

+{
+       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.

[...]

quoted

quoted

"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?

Will use fixedregulator for sdio case.
Thx!

[...]

Kind regards
Uffe